diff --git a/app/Http/Controllers/Api/ApiV1Controller.php b/app/Http/Controllers/Api/ApiV1Controller.php index c8e23fd86..52e7f9ac9 100644 --- a/app/Http/Controllers/Api/ApiV1Controller.php +++ b/app/Http/Controllers/Api/ApiV1Controller.php @@ -106,6 +106,14 @@ class ApiV1Controller extends Controller const PF_API_ENTITY_KEY = '_pe'; + /** + * How deep a home feed rebuild goes while a request is waiting for it. + * Several screens worth, rather than the full feed cap: the queued job + * takes it the rest of the way. Raising this means raising the warm + * lock's ttl with it. + */ + const HOME_COLD_BOOT_DEPTH = 100; + public function __construct() { $this->fractal = new Fractal\Manager; @@ -2776,6 +2784,57 @@ class ApiV1Controller extends Controller ]; } + /** + * Rebuild a home feed that has no cached set and serve the window the + * request asked for out of it. + * + * The settled-empty guard comes first, so a feed a completed warm has + * already found empty costs nothing to read. The lock is non-blocking and + * short-lived: a request that loses it serves nothing rather than running + * a second full warm, which is the concurrency protection the old + * check-then-set marker could not give. A warm that throws releases the + * lock and writes no record of emptiness, so the next request retries. + */ + protected function homeColdBoot($pid, array $cursor, int $paddedLimit): array + { + if (! HomeTimelineService::shouldWarmEmpty($pid)) { + return []; + } + + $lock = Cache::lock('pf:timeline:home:warm:lock:'.$pid, 30); + + if (! $lock->get()) { + return []; + } + + try { + $added = HomeTimelineService::warmCache($pid, true, self::HOME_COLD_BOOT_DEPTH); + } catch (\Throwable $e) { + Log::warning('timelineHome: home feed warm failed', [ + 'pid' => $pid, + 'error' => $e->getMessage(), + ]); + + return []; + } finally { + $lock->release(); + } + + if ($added === 0) { + return []; + } + + if ($added >= self::HOME_COLD_BOOT_DEPTH) { + // The warm filled the depth a request should wait for, so there + // is more to collect than this response needs. A shallower result + // means the candidate set was exhausted and a deeper pass over + // the same query cannot find more. + FeedWarmCachePipeline::dispatch($pid)->onQueue('feed'); + } + + return $this->fetchHomeWindow($pid, $cursor, $paddedLimit); + } + protected function fetchHomeWindow($pid, array $cursor, int $paddedLimit): array { if ($cursor['min']) { @@ -2846,25 +2905,12 @@ class ApiV1Controller extends Controller $ids = $this->fetchHomeWindow($pid, $cursor, $paddedLimit); - if (! $ids) { - if (HomeTimelineService::count($pid) > 0) { - return $this->json([]); - } - - $coldBootKey = 'pf:services:apiv1:home:cached:coldbootcheck:'.$pid; - - if (! Cache::has($coldBootKey)) { - Cache::set($coldBootKey, 1, 300); - FeedWarmCachePipeline::dispatchSync($pid); - - $ids = HomeTimelineService::get($pid, 0, $paddedLimit); - - if (! $ids) { - return $this->json([]); - } - } else { - return $this->json([]); - } + // No cached set at all. This fires for a cursor-ful request too: a + // client holding a cursor from a feed that has since gone used to get + // an empty page on every retry, because the rebuild only ran for a + // request that sent no cursor. + if (! HomeTimelineService::cacheKeyExists($pid)) { + $ids = $this->homeColdBoot($pid, $cursor, $paddedLimit); } $applyPhotosReblogsOnly = $photosReblogsOnly; diff --git a/tests/Feature/Timeline/HomeColdBootOnReadTest.php b/tests/Feature/Timeline/HomeColdBootOnReadTest.php new file mode 100644 index 000000000..8c1dcf04c --- /dev/null +++ b/tests/Feature/Timeline/HomeColdBootOnReadTest.php @@ -0,0 +1,185 @@ +create([ + 'profile_id' => $profileId, + 'type' => 'photo', + 'scope' => 'public', + 'visibility' => 'public', + 'local' => true, + 'in_reply_to_id' => null, + 'reblog_of_id' => null, + 'uri' => null, + 'created_at' => now()->subMinutes($minutesAgo), + ]); + + StatusService::get($status->id, false); + StatusService::getMastodon($status->id, false); + + return $status; +} + +/** A viewer with a cold feed and $count eligible posts from one follow. */ +function coldBootViewerWithPosts(int $count): array +{ + $viewer = User::factory()->create(); + $viewer->refresh(); + $author = User::factory()->create(); + $author->refresh(); + + Follower::withoutEvents(fn () => Follower::create([ + 'profile_id' => $viewer->profile_id, + 'following_id' => $author->profile_id, + ])); + + $statuses = collect(range(0, $count - 1))->map( + fn ($i) => coldBootPhoto($author->profile_id, $count - $i) + ); + + Redis::del(HomeTimelineService::CACHE_KEY.$viewer->profile_id); + + return [$viewer, $author, $statuses->sortByDesc(fn ($s) => (int) $s->id)->values()]; +} + +function coldBootStatusQueries(callable $fn): int +{ + $count = 0; + + DB::listen(function ($query) use (&$count) { + if (preg_match('/from\s+"?statuses"?/i', $query->sql)) { + $count++; + } + }); + + $fn(); + + return $count; +} + +it('honours the cursor on the page served by a cold boot', function () { + [$viewer, , $ordered] = coldBootViewerWithPosts(12); + + expect(HomeTimelineService::cacheKeyExists($viewer->profile_id))->toBeFalse(); + + $cursor = (int) $ordered->get(8)->id; + + Passport::actingAs($viewer, ['read']); + + $res = $this->getJson('/api/v1/timelines/home?limit=4&max_id='.$cursor) + ->assertOk() + ->json(); + + $ids = collect($res)->pluck('id')->map(fn ($id) => (int) $id)->all(); + + expect($ids)->not->toBeEmpty(); + + foreach ($ids as $id) { + expect($id)->toBeLessThan($cursor); + } +}); + +it('does not rebuild while another request holds the warm lock', function () { + [$viewer] = coldBootViewerWithPosts(4); + $pid = $viewer->profile_id; + + $held = Cache::lock(coldBootLockKey($pid), 30); + expect($held->get())->toBeTrue(); + + Passport::actingAs($viewer, ['read']); + + $queries = coldBootStatusQueries(function () { + $this->getJson('/api/v1/timelines/home?limit=4')->assertOk(); + }); + + expect($queries)->toBe(0); + + $held->release(); +}); + +it('releases the warm lock it took', function () { + [$viewer] = coldBootViewerWithPosts(4); + $pid = $viewer->profile_id; + + Passport::actingAs($viewer, ['read']); + + $this->getJson('/api/v1/timelines/home?limit=4')->assertOk(); + + $lock = Cache::lock(coldBootLockKey($pid), 30); + + expect($lock->get())->toBeTrue(); + + $lock->release(); +}); + +it('leaves the feed rebuildable when the rebuild fails', function () { + [$viewer, , $ordered] = coldBootViewerWithPosts(3); + $pid = $viewer->profile_id; + + Passport::actingAs($viewer, ['read']); + + Schema::rename('statuses', 'statuses_unavailable'); + + try { + $first = $this->getJson('/api/v1/timelines/home?limit=4'); + } finally { + Schema::rename('statuses_unavailable', 'statuses'); + } + + $first->assertOk(); + expect($first->json())->toBe([]) + ->and(HomeTimelineService::shouldWarmEmpty($pid))->toBeTrue(); + + $ids = collect($this->getJson('/api/v1/timelines/home?limit=4')->assertOk()->json()) + ->pluck('id')->map(fn ($id) => (int) $id)->all(); + + expect($ids)->toContain((int) $ordered->first()->id); +}); + +it('completes a cold boot over a large candidate set promptly', function () { + [$viewer] = coldBootViewerWithPosts(120); + + Passport::actingAs($viewer, ['read']); + + $start = microtime(true); + $this->getJson('/api/v1/timelines/home?limit=20')->assertOk(); + $elapsed = (microtime(true) - $start) * 1000; + + expect($elapsed)->toBeLessThan(3000); +}); diff --git a/tests/Feature/Timeline/HomeColdStartServesContentTest.php b/tests/Feature/Timeline/HomeColdStartServesContentTest.php index b36921549..d51bbd7ad 100644 --- a/tests/Feature/Timeline/HomeColdStartServesContentTest.php +++ b/tests/Feature/Timeline/HomeColdStartServesContentTest.php @@ -60,4 +60,8 @@ it('returns 200 empty when a cold feed warms to nothing', function () { $res = $this->getJson('/api/v1/timelines/home')->assertOk(); expect($res->json())->toBe([]); + + // The warm completed and found nothing, so it is recorded as empty + // rather than left looking like a feed that needs building. + expect(HomeTimelineService::shouldWarmEmpty($pid))->toBeFalse(); }); diff --git a/tests/Feature/Timeline/HomeEmptyCursorTest.php b/tests/Feature/Timeline/HomeEmptyCursorTest.php index 245312a04..eb42acec1 100644 --- a/tests/Feature/Timeline/HomeEmptyCursorTest.php +++ b/tests/Feature/Timeline/HomeEmptyCursorTest.php @@ -47,7 +47,7 @@ it('returns 200 with no cold-boot when a populated feed has nothing newer than m // A populated-but-empty-window read must not trip cold-boot warming. Bus::assertNotDispatched(FeedWarmCachePipeline::class); - expect(Cache::has('pf:services:apiv1:home:cached:coldbootcheck:'.$pid))->toBeFalse(); + expect(HomeTimelineService::shouldWarmEmpty($pid))->toBeTrue(); }); it('warms once on a genuinely cold feed and guards re-warming', function () { @@ -61,8 +61,13 @@ it('warms once on a genuinely cold feed and guards re-warming', function () { $this->getJson('/api/v1/timelines/home')->assertOk(); - Bus::assertDispatchedSync(FeedWarmCachePipeline::class, 1); + // The warm ran in-request and found nothing, so it is recorded and no + // deepening job was queued. + expect(HomeTimelineService::shouldWarmEmpty($pid))->toBeFalse(); + Bus::assertNotDispatched(FeedWarmCachePipeline::class); - $ttl = Cache::has('pf:services:apiv1:home:cached:coldbootcheck:'.$pid); - expect($ttl)->toBeTrue(); + // And the warm lock was released rather than left held for its ttl. + $lock = Cache::lock('pf:timeline:home:warm:lock:'.$pid, 30); + expect($lock->get())->toBeTrue(); + $lock->release(); }); diff --git a/tests/Feature/Timeline/HomeTimelineApiTest.php b/tests/Feature/Timeline/HomeTimelineApiTest.php index 8028b615f..f4415c21a 100644 --- a/tests/Feature/Timeline/HomeTimelineApiTest.php +++ b/tests/Feature/Timeline/HomeTimelineApiTest.php @@ -8,7 +8,7 @@ use App\Services\HomeTimelineService; use App\Services\StatusService; use Illuminate\Foundation\Testing\LazilyRefreshDatabase; use Illuminate\Support\Facades\Bus; -use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Redis; use Laravel\Passport\Passport; @@ -26,15 +26,9 @@ uses(LazilyRefreshDatabase::class); | */ -function homeColdBootKey(int $pid): string -{ - return 'pf:services:apiv1:home:cached:coldbootcheck:'.$pid; -} - function flushHome(int $pid): void { Redis::del(HomeTimelineService::CACHE_KEY.$pid); - Cache::forget(homeColdBootKey($pid)); } /** @@ -121,19 +115,30 @@ it('warms once on cold-start and does not warm again', function () { Passport::actingAs($viewer, ['read']); + $candidateQueries = 0; + DB::listen(function ($query) use (&$candidateQueries) { + if (preg_match('/from\s+"?statuses"?/i', $query->sql)) { + $candidateQueries++; + } + }); + $first = $this->getJson('/api/v1/timelines/home'); $first->assertOk(); expect($first->json())->toBe([]); + expect($candidateQueries)->toBeGreaterThan(0); - Bus::assertDispatchedSync(FeedWarmCachePipeline::class, 1); + // Second immediate request: still empty, and the record of the completed + // empty warm must stop it querying for candidates again. + $candidateQueries = 0; - // Second immediate request: still empty, and the coldBootKey guard must - // stop a second warm from dispatching. $second = $this->getJson('/api/v1/timelines/home'); $second->assertOk(); expect($second->json())->toBe([]); + expect($candidateQueries)->toBe(0); - Bus::assertDispatchedSync(FeedWarmCachePipeline::class, 1); + // The queued deepening job only runs when the warm filled its depth. + Bus::assertNotDispatched(FeedWarmCachePipeline::class); + expect(HomeTimelineService::shouldWarmEmpty($pid))->toBeFalse(); flushHome($pid); }); diff --git a/tests/Feature/Timeline/TimelineScrollPaginationTest.php b/tests/Feature/Timeline/TimelineScrollPaginationTest.php index 87c40c19a..0a14e14df 100644 --- a/tests/Feature/Timeline/TimelineScrollPaginationTest.php +++ b/tests/Feature/Timeline/TimelineScrollPaginationTest.php @@ -9,7 +9,6 @@ use App\Services\PublicTimelineService; use App\Services\StatusService; use Carbon\Carbon; use Illuminate\Foundation\Testing\LazilyRefreshDatabase; -use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Redis; use Laravel\Passport\Passport; @@ -90,7 +89,6 @@ it('paginates the home feed without duplicates when id order differs from date o } Redis::del(HomeTimelineService::CACHE_KEY.$pid); - Cache::forget('pf:services:apiv1:home:cached:coldbootcheck:'.$pid); HomeTimelineService::warmCache($pid, true, 400); $cached = collect(Redis::zrevrange(HomeTimelineService::CACHE_KEY.$pid, 0, -1)) @@ -206,7 +204,6 @@ it('advances the home feed when the client rounds the snowflake cursor', functio } Redis::del(HomeTimelineService::CACHE_KEY.$pid); - Cache::forget('pf:services:apiv1:home:cached:coldbootcheck:'.$pid); HomeTimelineService::warmCache($pid, true, 400); $ordered = collect(Redis::zrevrange(HomeTimelineService::CACHE_KEY.$pid, 0, -1))