From 896ea6f33122bcb30582a318b5d2e09b40a9ca53 Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Sun, 1 Nov 2026 11:25:02 +0100 Subject: [PATCH 1/3] feat: import every user's GitHub follows from a queued job, reading their stored handle --- app/Actions/CreateUserGitHubFollows.php | 8 +- .../Commands/ImportGitHubFollowsCommand.php | 71 +--------- app/Jobs/ImportAllGitHubFollowsJob.php | 95 ++++++++++++++ .../Commands/ImportGitHubFollowsTest.php | 117 +---------------- .../Actions/CreateUserGitHubFollowsTest.php | 21 ++- .../Jobs/ImportAllGitHubFollowsJobTest.php | 121 ++++++++++++++++++ .../Unit/Jobs/ImportGitHubFollowsJobTest.php | 1 - 7 files changed, 241 insertions(+), 193 deletions(-) create mode 100644 app/Jobs/ImportAllGitHubFollowsJob.php create mode 100644 tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php diff --git a/app/Actions/CreateUserGitHubFollows.php b/app/Actions/CreateUserGitHubFollows.php index fc5473fec..688b63176 100644 --- a/app/Actions/CreateUserGitHubFollows.php +++ b/app/Actions/CreateUserGitHubFollows.php @@ -40,10 +40,12 @@ public function handle(User $user, bool $notify = true): void return; } - $login = $this->github->login($user->github_id); - $followingIds = $login === null ? ['users' => [], 'organizations' => []] : $this->github->followingIds($login, self::MaxPages); + // The handle is the GitHub login as of the user's last sign-in, which saves looking it up by ID: a sign-in + // updates it right before importing. An account renamed since then isn't found, or in the rare case someone took + // its old handle, has theirs read instead. + $followingIds = $this->github->followingIds($user->handle, self::MaxPages); $following = $importsPeople ? $this->users($user, $followingIds['users']) : new Collection(); - $followers = $importsPeople && $login !== null ? $this->users($user, $this->github->followerIds($login, self::MaxPages)) : new Collection(); + $followers = $importsPeople ? $this->users($user, $this->github->followerIds($user->handle, self::MaxPages)) : new Collection(); $organizationIds = collect($followingIds['organizations'])->unique()->values(); DB::transaction(function () use ($user, $notify, $following, $followers, $organizationIds, $importsPeople, $importsOrganizations): void { diff --git a/app/Console/Commands/ImportGitHubFollowsCommand.php b/app/Console/Commands/ImportGitHubFollowsCommand.php index 225446192..758d1a364 100644 --- a/app/Console/Commands/ImportGitHubFollowsCommand.php +++ b/app/Console/Commands/ImportGitHubFollowsCommand.php @@ -4,80 +4,21 @@ namespace App\Console\Commands; -use App\Actions\CreateUserGitHubFollows; -use App\Contracts\Services\GitHubProvider; -use App\Models\User; +use App\Jobs\ImportAllGitHubFollowsJob; use Illuminate\Console\Attributes\Description; use Illuminate\Console\Attributes\Signature; use Illuminate\Console\Command; -use Illuminate\Database\Eloquent\Builder; -use Illuminate\Http\Client\ConnectionException; -use Illuminate\Http\Client\RequestException; #[Signature('follows:import')] -#[Description('Follow, for every user not imported yet, who and which organizations they follow and who follows them on GitHub, without notifying anyone')] +#[Description('Queue the import of who and which organizations every user not imported yet follows, and who follows them, on GitHub, without notifying anyone')] final class ImportGitHubFollowsCommand extends Command { - /** - * Below this many requests left, the command stops, leaving the rest to the app until the rate limit resets: above - * the floor where looking up mentioned organizations pauses, so they carry on meanwhile. - */ - private const int RateLimitFloor = 1500; - - public function handle(CreateUserGitHubFollows $createUserGitHubFollows, GitHubProvider $github): int + public function handle(): int { - $imported = 0; - $failed = 0; - - foreach ($this->users()->lazyById() as $user) { - $rateLimit = $github->rateLimit(); - - // A user takes one request for their login, and up to the maximum of pages each way. - if ($rateLimit !== null && $rateLimit['remaining'] < self::RateLimitFloor + 1 + 2 * CreateUserGitHubFollows::MaxPages) { - $this->components->warn("Stopped after {$imported} users, to keep ".self::RateLimitFloor." GitHub requests for the rest of the app: {$rateLimit['remaining']} left."); - $this->components->info("{$this->users()->count()} users left. Run the command again after {$rateLimit['resets_at']->utc()->toDateTimeString()} UTC ({$rateLimit['resets_at']->diffForHumans()}), when the GitHub rate limit resets."); - - return self::FAILURE; - } - - try { - $createUserGitHubFollows->handle($user, notify: false); - - $imported++; - } catch (RequestException $exception) { - if (in_array($exception->response->status(), [403, 429], true)) { - $this->components->error("GitHub rate limit reached after {$imported} users. Run the command again later."); - - return self::FAILURE; - } - - $this->failed($user, $exception); - - $failed++; - } catch (ConnectionException $exception) { - $this->failed($user, $exception); + dispatch(new ImportAllGitHubFollowsJob()); - $failed++; - } - } - - $this->components->info("Imported the GitHub follows of {$imported} users."); - - return $failed === 0 ? self::SUCCESS : self::FAILURE; - } - - /** - * @return Builder - */ - private function users(): Builder - { - return User::query()->where(fn (Builder $query): Builder => $query->whereNull('github_follows_imported_at')->orWhereNull('github_organization_follows_imported_at')); - } - - private function failed(User $user, RequestException|ConnectionException $exception): void - { - report($exception); + $this->components->info('Queued the import, '.ImportAllGitHubFollowsJob::BatchSize.' users at a time. It waits for the GitHub rate limit to reset by itself, keeping '.ImportAllGitHubFollowsJob::Reserve.' requests for the rest of the app, and logs when every user is imported.'); - $this->components->warn("Could not import the GitHub follows of {$user->mention()}."); + return self::SUCCESS; } } diff --git a/app/Jobs/ImportAllGitHubFollowsJob.php b/app/Jobs/ImportAllGitHubFollowsJob.php new file mode 100644 index 000000000..104ad0213 --- /dev/null +++ b/app/Jobs/ImportAllGitHubFollowsJob.php @@ -0,0 +1,95 @@ +rateLimit(); + + // A user takes up to the maximum of pages each way. + if ($rateLimit !== null && $rateLimit['remaining'] < self::Reserve + self::BatchSize * 2 * CreateUserGitHubFollows::MaxPages) { + // A minute past the reset, so GitHub counts from the new hour by then. + $this->continueAt($rateLimit['resets_at']->addMinute()); + + return; + } + + $users = User::query() + ->where(fn (Builder $query): Builder => $query->whereNull('github_follows_imported_at')->orWhereNull('github_organization_follows_imported_at')) + ->when($this->after !== null, fn (Builder $query): Builder => $query->where('id', '>', $this->after)) + ->orderBy('id') + ->limit(self::BatchSize) + ->get(); + + foreach ($users as $user) { + try { + $createUserGitHubFollows->handle($user, notify: false); + } catch (RequestException $exception) { + if (in_array($exception->response->status(), [403, 429], true)) { + $this->continueAt(now()->addMinutes(15)); + + return; + } + + // An account renamed since its last sign-in isn't found under its handle: it is imported at its next one. + report_if($exception->response->status() !== 404, $exception); + } catch (ConnectionException $exception) { + report($exception); + } + + $this->after = $user->id; + } + + if ($users->count() < self::BatchSize) { + Log::info('Imported the GitHub follows of every user.'); + + return; + } + + $this->continueAt(now()); + } + + /** + * Some queues can't hold a job back more than 15 minutes, so a longer wait reads the rate limit again by then. + */ + private function continueAt(CarbonInterface $at): void + { + dispatch(new self($this->after))->delay($at->min(now()->addMinutes(15))); + } +} diff --git a/tests/Feature/Commands/ImportGitHubFollowsTest.php b/tests/Feature/Commands/ImportGitHubFollowsTest.php index f9b6a1051..b0dad514c 100644 --- a/tests/Feature/Commands/ImportGitHubFollowsTest.php +++ b/tests/Feature/Commands/ImportGitHubFollowsTest.php @@ -2,120 +2,15 @@ declare(strict_types=1); -use App\Contracts\Services\GitHubProvider; -use App\Models\Notification; -use App\Models\Organization; -use App\Models\User; -use Carbon\CarbonImmutable; -use GuzzleHttp\Psr7\Response as Psr7Response; -use Illuminate\Http\Client\ConnectionException; -use Illuminate\Http\Client\RequestException; -use Illuminate\Http\Client\Response; -use Illuminate\Support\Facades\Exceptions; +use App\Jobs\ImportAllGitHubFollowsJob; +use Illuminate\Support\Facades\Queue; -it('imports the github follows of every user not imported yet, without notifying anyone', function (): void { - $alice = User::factory()->create(); - $bob = User::factory()->create(); - $imported = User::factory()->create(['github_follows_imported_at' => now()->subDay(), 'github_organization_follows_imported_at' => now()->subDay()]); - - $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('rateLimit')->andReturnNull(); - $github->shouldReceive('login')->with($alice->github_id)->andReturn($alice->handle); - $github->shouldReceive('followingIds')->with($alice->handle, 10)->andReturn(['users' => [$bob->github_id, $imported->github_id], 'organizations' => []]); - $github->shouldReceive('followerIds')->with($alice->handle, 10)->andReturn([]); - $github->shouldReceive('login')->with($bob->github_id)->andReturn($bob->handle); - $github->shouldReceive('followingIds')->with($bob->handle, 10)->andReturn(['users' => [], 'organizations' => []]); - $github->shouldReceive('followerIds')->with($bob->handle, 10)->andReturn([$alice->github_id]); - - $this->artisan('follows:import') - ->expectsOutputToContain('Imported the GitHub follows of 2 users.') - ->assertSuccessful(); - - expect($alice->isFollowing($bob))->toBeTrue() - ->and($alice->isFollowing($imported))->toBeTrue() - ->and($bob->refresh()->github_follows_imported_at)->not->toBeNull() - ->and(Notification::query()->count())->toBe(0); - - $github->shouldNotHaveReceived('login', [$imported->github_id]); -}); - -it('keeps going when the follows of a user cannot be imported', function (string $exception, Closure $throwable): void { - Exceptions::fake(); - - $failing = User::factory()->create(['handle' => 'failing']); - $user = User::factory()->create(); - - $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('rateLimit')->andReturnNull(); - $github->shouldReceive('login')->with($failing->github_id)->andThrow($throwable()); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); - $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [], 'organizations' => []]); - $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([]); - - $this->artisan('follows:import') - ->expectsOutputToContain('Could not import the GitHub follows of @failing.') - ->expectsOutputToContain('Imported the GitHub follows of 1 users.') - ->assertFailed(); - - expect($failing->refresh()->github_follows_imported_at)->toBeNull() - ->and($user->refresh()->github_follows_imported_at)->not->toBeNull(); - - Exceptions::assertReported($exception); -})->with([ - 'server error' => [RequestException::class, fn (): RequestException => new RequestException(new Response(new Psr7Response(500)))], - 'connection error' => [ConnectionException::class, fn (): ConnectionException => new ConnectionException()], -]); - -it('stops when github rate limits the requests', function (int $status): void { - $user = User::factory()->create(); - User::factory()->create(); - - $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('rateLimit')->andReturnNull(); - $github->shouldReceive('login')->with($user->github_id)->once()->andThrow(new RequestException(new Response(new Psr7Response($status)))); - - $this->artisan('follows:import') - ->expectsOutputToContain('GitHub rate limit reached after 0 users. Run the command again later.') - ->assertFailed(); - - $github->shouldHaveReceived('login')->once(); -})->with([403, 429]); - -it('imports the organizations followed by the users whose people were imported before', function (): void { - $user = User::factory()->create(['github_follows_imported_at' => now()->subDay()]); - $organization = Organization::factory()->create(); - - $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('rateLimit')->andReturnNull(); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); - $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [], 'organizations' => [$organization->profile->github_id]]); +it('queues the import of every user not imported yet', function (): void { + Queue::fake(); $this->artisan('follows:import') - ->expectsOutputToContain('Imported the GitHub follows of 1 users.') + ->expectsOutputToContain('Queued the import, 25 users at a time. It waits for the GitHub rate limit to reset by itself, keeping 1500 requests for the rest of the app') ->assertSuccessful(); - expect($user->followedOrganizations()->pluck('organizations.id')->all())->toBe([$organization->id]); -}); - -it('stops before using up the github requests the rest of the app needs, and says when to run it again', function (): void { - $this->travelTo(CarbonImmutable::parse('2026-11-01 10:00:00', 'UTC')); - - $imported = User::factory()->create(); - User::factory()->count(2)->create(); - - $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('rateLimit')->andReturn( - ['remaining' => 1521, 'limit' => 5000, 'resets_at' => CarbonImmutable::parse('2026-11-01 10:42:00', 'UTC')], - ['remaining' => 1520, 'limit' => 5000, 'resets_at' => CarbonImmutable::parse('2026-11-01 10:42:00', 'UTC')], - ); - $github->shouldReceive('login')->with($imported->github_id)->once()->andReturn($imported->handle); - $github->shouldReceive('followingIds')->with($imported->handle, 10)->andReturn(['users' => [], 'organizations' => []]); - $github->shouldReceive('followerIds')->with($imported->handle, 10)->andReturn([]); - - $this->artisan('follows:import') - ->expectsOutputToContain('Stopped after 1 users, to keep 1500 GitHub requests for the rest of the app: 1520 left.') - ->expectsOutputToContain('2 users left. Run the command again after 2026-11-01 10:42:00 UTC (42 minutes from now), when the GitHub rate limit resets.') - ->assertFailed(); - - $github->shouldHaveReceived('login')->once(); + Queue::assertPushed(ImportAllGitHubFollowsJob::class, fn (ImportAllGitHubFollowsJob $job): bool => $job->after === null); }); diff --git a/tests/Unit/Actions/CreateUserGitHubFollowsTest.php b/tests/Unit/Actions/CreateUserGitHubFollowsTest.php index e39ad7835..2c5df97f1 100644 --- a/tests/Unit/Actions/CreateUserGitHubFollowsTest.php +++ b/tests/Unit/Actions/CreateUserGitHubFollowsTest.php @@ -19,7 +19,6 @@ $notFollowed = User::factory()->create(); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [$followed->github_id], 'organizations' => []]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([]); @@ -35,7 +34,6 @@ $follower = User::factory()->create(); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [], 'organizations' => []]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([$follower->github_id]); @@ -50,7 +48,6 @@ $stranger = User::factory()->make(['github_id' => '999']); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [$stranger->github_id], 'organizations' => []]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([$stranger->github_id]); @@ -66,7 +63,6 @@ $follower = User::factory()->create(); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [$followed->github_id], 'organizations' => []]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([$follower->github_id]); @@ -86,7 +82,6 @@ $follower = User::factory()->create(); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [$followed->github_id], 'organizations' => []]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([$follower->github_id]); @@ -105,7 +100,6 @@ $user->following()->attach([$alreadyFollowed->id, $notOnGitHub->id]); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [$alreadyFollowed->github_id], 'organizations' => []]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([]); @@ -120,13 +114,18 @@ $user = User::factory()->create(['github_follows_imported_at' => now(), 'github_organization_follows_imported_at' => now()]); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->never(); + $github->shouldReceive('followingIds')->never(); resolve(CreateUserGitHubFollows::class)->handle($user); }); -it('marks the follows as imported when the github account no longer exists', function (): void { - $user = User::factory()->create(); +it('reads the github accounts under the handle the user last signed in with, without looking it up', function (): void { + $user = User::factory()->create(['handle' => 'octocat']); + + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('followingIds')->with('octocat', 10)->once()->andReturn(['users' => [], 'organizations' => []]); + $github->shouldReceive('followerIds')->with('octocat', 10)->once()->andReturn([]); + $github->shouldReceive('login')->never(); resolve(CreateUserGitHubFollows::class)->handle($user); @@ -139,7 +138,6 @@ $user = User::factory()->create(); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->andThrow(new RequestException(new Response(new Psr7Response(500)))); expect(fn () => resolve(CreateUserGitHubFollows::class)->handle($user))->toThrow(RequestException::class) @@ -153,7 +151,6 @@ Organization::factory()->create(); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [], 'organizations' => [$organization->profile->github_id, '958072']]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([]); @@ -172,7 +169,6 @@ $organization = Organization::factory()->create(); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [$unfollowed->github_id], 'organizations' => [$organization->profile->github_id]]); resolve(CreateUserGitHubFollows::class)->handle($user); @@ -195,7 +191,6 @@ $blocker->blocking()->attach($user); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [$blocked->github_id, $blocker->github_id, $allowed->github_id], 'organizations' => []]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([$blocked->github_id, $blocker->github_id, $allowed->github_id]); diff --git a/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php b/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php new file mode 100644 index 000000000..9457aa827 --- /dev/null +++ b/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php @@ -0,0 +1,121 @@ +create(['handle' => 'alice']); + $bob = User::factory()->create(['handle' => 'bob', 'github_follows_imported_at' => now()->subDay()]); + $imported = User::factory()->create(['github_follows_imported_at' => now()->subDay(), 'github_organization_follows_imported_at' => now()->subDay()]); + $organization = Organization::factory()->create(); + + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('rateLimit')->andReturnNull(); + $github->shouldReceive('followingIds')->with('alice', 10)->andReturn(['users' => [$bob->github_id], 'organizations' => []]); + $github->shouldReceive('followerIds')->with('alice', 10)->andReturn([]); + $github->shouldReceive('followingIds')->with('bob', 10)->andReturn(['users' => [], 'organizations' => [$organization->profile->github_id]]); + + new ImportAllGitHubFollowsJob()->handle($github, resolve(CreateUserGitHubFollows::class)); + + expect($alice->isFollowing($bob))->toBeTrue() + ->and($bob->followedOrganizations()->pluck('organizations.id')->all())->toBe([$organization->id]) + ->and(Notification::query()->count())->toBe(0); + + $github->shouldNotHaveReceived('followingIds', [$imported->handle, 10]); + Queue::assertNothingPushed(); + Log::shouldHaveReceived('info')->with('Imported the GitHub follows of every user.')->once(); +}); + +it('queues the next batch after a full one, from the last user it read', function (): void { + Queue::fake(); + + $users = User::factory()->count(ImportAllGitHubFollowsJob::BatchSize + 1)->create()->sortBy('id')->values(); + + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('rateLimit')->andReturnNull(); + $github->shouldReceive('followingIds')->andReturn(['users' => [], 'organizations' => []]); + $github->shouldReceive('followerIds')->andReturn([]); + + new ImportAllGitHubFollowsJob()->handle($github, resolve(CreateUserGitHubFollows::class)); + + expect($users->last()->refresh()->github_follows_imported_at)->toBeNull(); + Queue::assertPushed(ImportAllGitHubFollowsJob::class, fn (ImportAllGitHubFollowsJob $job): bool => $job->after === $users[ImportAllGitHubFollowsJob::BatchSize - 1]->id); +}); + +it('waits for the rate limit to reset, at most 15 minutes at a time, keeping requests for the rest of the app', function (int $remaining, int $resetsInMinutes, int $delayMinutes): void { + Queue::fake(); + $this->freezeSecond(); + + User::factory()->create(); + + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('rateLimit')->andReturn(['remaining' => $remaining, 'limit' => 5000, 'resets_at' => CarbonImmutable::now()->addMinutes($resetsInMinutes)]); + $github->shouldReceive('followingIds')->never(); + + new ImportAllGitHubFollowsJob('previous')->handle($github, resolve(CreateUserGitHubFollows::class)); + + Queue::assertPushed(ImportAllGitHubFollowsJob::class, fn (ImportAllGitHubFollowsJob $job): bool => $job->after === 'previous' + && $job->delay instanceof DateTimeInterface + && CarbonImmutable::instance($job->delay)->equalTo(now()->addMinutes($delayMinutes))); +})->with([ + 'reset soon' => [1999, 5, 6], + 'reset in an hour' => [1000, 60, 15], +]); + +it('waits 15 minutes when github rate limits it, and imports that user then', function (int $status): void { + Queue::fake(); + $this->freezeSecond(); + + [$imported, $limited] = User::factory()->count(2)->create()->sortBy('id')->values(); + + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('rateLimit')->andReturnNull(); + $github->shouldReceive('followingIds')->with($imported->handle, 10)->andReturn(['users' => [], 'organizations' => []]); + $github->shouldReceive('followerIds')->andReturn([]); + $github->shouldReceive('followingIds')->with($limited->handle, 10)->andThrow(new RequestException(new Response(new Psr7Response($status)))); + + new ImportAllGitHubFollowsJob()->handle($github, resolve(CreateUserGitHubFollows::class)); + + expect($limited->refresh()->github_follows_imported_at)->toBeNull(); + Queue::assertPushed(ImportAllGitHubFollowsJob::class, fn (ImportAllGitHubFollowsJob $job): bool => $job->after === $imported->id + && CarbonImmutable::instance($job->delay)->equalTo(now()->addMinutes(15))); +})->with([403, 429]); + +it('skips the users github fails on, reporting all but the accounts renamed since their last sign-in', function (): void { + Queue::fake(); + Exceptions::fake(); + + [$renamed, $failing, $unreachable, $user] = User::factory()->count(4)->create()->sortBy('id')->values(); + + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('rateLimit')->andReturnNull(); + $github->shouldReceive('followingIds')->with($renamed->handle, 10)->andThrow(new RequestException(new Response(new Psr7Response(404)))); + $github->shouldReceive('followingIds')->with($failing->handle, 10)->andThrow(new RequestException(new Response(new Psr7Response(500)))); + $github->shouldReceive('followingIds')->with($unreachable->handle, 10)->andThrow(new ConnectionException()); + $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [], 'organizations' => []]); + $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([]); + + new ImportAllGitHubFollowsJob()->handle($github, resolve(CreateUserGitHubFollows::class)); + + expect($renamed->refresh()->github_follows_imported_at)->toBeNull() + ->and($user->refresh()->github_follows_imported_at)->not->toBeNull(); + + Exceptions::assertReportedCount(2); +}); diff --git a/tests/Unit/Jobs/ImportGitHubFollowsJobTest.php b/tests/Unit/Jobs/ImportGitHubFollowsJobTest.php index 212c660c9..b1e7dc5a4 100644 --- a/tests/Unit/Jobs/ImportGitHubFollowsJobTest.php +++ b/tests/Unit/Jobs/ImportGitHubFollowsJobTest.php @@ -12,7 +12,6 @@ $followed = User::factory()->create(); $github = $this->mock(GitHubProvider::class); - $github->shouldReceive('login')->with($user->github_id)->andReturn($user->handle); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [$followed->github_id], 'organizations' => []]); $github->shouldReceive('followerIds')->with($user->handle, 10)->andReturn([]); From 8c59f71ec6741db5b1cee55ca7c271615fdc1ead Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Sun, 1 Nov 2026 11:28:06 +0100 Subject: [PATCH 2/3] fix: import nothing for the bots, whose handles belong to strangers on GitHub --- app/Actions/CreateUserGitHubFollows.php | 7 ++++--- app/Jobs/ImportAllGitHubFollowsJob.php | 2 +- .../Unit/Actions/CreateUserGitHubFollowsTest.php | 16 ++++++++++++++++ .../Unit/Jobs/ImportAllGitHubFollowsJobTest.php | 2 +- 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/app/Actions/CreateUserGitHubFollows.php b/app/Actions/CreateUserGitHubFollows.php index 688b63176..f2270c9c9 100644 --- a/app/Actions/CreateUserGitHubFollows.php +++ b/app/Actions/CreateUserGitHubFollows.php @@ -42,10 +42,11 @@ public function handle(User $user, bool $notify = true): void // The handle is the GitHub login as of the user's last sign-in, which saves looking it up by ID: a sign-in // updates it right before importing. An account renamed since then isn't found, or in the rare case someone took - // its old handle, has theirs read instead. - $followingIds = $this->github->followingIds($user->handle, self::MaxPages); + // its old handle, has theirs read instead. The bots' handles belong to strangers on GitHub, so they import nothing. + $isBot = in_array($user->github_id, User::BOT_GITHUB_IDS, true); + $followingIds = $isBot ? ['users' => [], 'organizations' => []] : $this->github->followingIds($user->handle, self::MaxPages); $following = $importsPeople ? $this->users($user, $followingIds['users']) : new Collection(); - $followers = $importsPeople ? $this->users($user, $this->github->followerIds($user->handle, self::MaxPages)) : new Collection(); + $followers = $importsPeople && ! $isBot ? $this->users($user, $this->github->followerIds($user->handle, self::MaxPages)) : new Collection(); $organizationIds = collect($followingIds['organizations'])->unique()->values(); DB::transaction(function () use ($user, $notify, $following, $followers, $organizationIds, $importsPeople, $importsOrganizations): void { diff --git a/app/Jobs/ImportAllGitHubFollowsJob.php b/app/Jobs/ImportAllGitHubFollowsJob.php index 104ad0213..436283404 100644 --- a/app/Jobs/ImportAllGitHubFollowsJob.php +++ b/app/Jobs/ImportAllGitHubFollowsJob.php @@ -77,7 +77,7 @@ public function handle(GitHubProvider $github, CreateUserGitHubFollows $createUs } if ($users->count() < self::BatchSize) { - Log::info('Imported the GitHub follows of every user.'); + Log::info('Went through every user whose GitHub follows were not imported yet.'); return; } diff --git a/tests/Unit/Actions/CreateUserGitHubFollowsTest.php b/tests/Unit/Actions/CreateUserGitHubFollowsTest.php index 2c5df97f1..d585b1d5e 100644 --- a/tests/Unit/Actions/CreateUserGitHubFollowsTest.php +++ b/tests/Unit/Actions/CreateUserGitHubFollowsTest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use App\Actions\CreateTomFriendship; use App\Actions\CreateUserGitHubFollows; use App\Contracts\Services\GitHubProvider; use App\Enums\NotificationType; @@ -134,6 +135,21 @@ ->github_organization_follows_imported_at->not->toBeNull(); }); +it('imports nothing for the bots, whose handles belong to strangers on github', function (): void { + resolve(CreateTomFriendship::class)->handle(User::factory()->create()); + $tom = User::query()->where('github_id', CreateTomFriendship::GITHUB_ID)->sole(); + + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('followingIds')->never(); + $github->shouldReceive('followerIds')->never(); + + resolve(CreateUserGitHubFollows::class)->handle($tom); + + expect($tom->refresh()) + ->github_follows_imported_at->not->toBeNull() + ->github_organization_follows_imported_at->not->toBeNull(); +}); + it('leaves the follows to import again when github fails', function (): void { $user = User::factory()->create(); diff --git a/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php b/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php index 9457aa827..ed9cf1b12 100644 --- a/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php +++ b/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php @@ -40,7 +40,7 @@ $github->shouldNotHaveReceived('followingIds', [$imported->handle, 10]); Queue::assertNothingPushed(); - Log::shouldHaveReceived('info')->with('Imported the GitHub follows of every user.')->once(); + Log::shouldHaveReceived('info')->with('Went through every user whose GitHub follows were not imported yet.')->once(); }); it('queues the next batch after a full one, from the last user it read', function (): void { From 80e1af04a477a598b724a8526ee42336daa054b8 Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Sun, 1 Nov 2026 12:57:03 +0100 Subject: [PATCH 3/3] fix: bound each import job's duration, and only wait on GitHub 403s that are rate limits --- app/Jobs/ImportAllGitHubFollowsJob.php | 33 ++++++++++++++- .../Jobs/ImportAllGitHubFollowsJobTest.php | 40 ++++++++++++++++--- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/app/Jobs/ImportAllGitHubFollowsJob.php b/app/Jobs/ImportAllGitHubFollowsJob.php index 436283404..057322213 100644 --- a/app/Jobs/ImportAllGitHubFollowsJob.php +++ b/app/Jobs/ImportAllGitHubFollowsJob.php @@ -14,13 +14,18 @@ use Illuminate\Foundation\Queue\Queueable; use Illuminate\Http\Client\ConnectionException; use Illuminate\Http\Client\RequestException; +use Illuminate\Http\Client\Response; +use Illuminate\Queue\Attributes\Timeout; use Illuminate\Support\Facades\Log; /** * Imports the GitHub follows of every user not imported yet, a few users at a time, without notifying anyone: each job * queues the next one, after the users it read, so a run takes no worker for long, survives deploys and waits for the * GitHub rate limit to reset by itself. It is unique, so launching it twice doesn't run it twice. + * + * Its timeout leaves room for the budget plus the longest a single user can take, under the queue's retry_after. */ +#[Timeout(300)] final class ImportAllGitHubFollowsJob implements ShouldBeUniqueUntilProcessing, ShouldQueue { use Queueable; @@ -33,6 +38,12 @@ final class ImportAllGitHubFollowsJob implements ShouldBeUniqueUntilProcessing, */ public const int Reserve = 1500; + /** + * The seconds a job spends importing before it leaves the rest to the next one, so it ends well within the worker's + * timeout: a user rarely takes a second, but one following thousands takes up to twenty requests. + */ + public const int Budget = 30; + public function __construct(public ?string $after = null) { // @@ -57,12 +68,15 @@ public function handle(GitHubProvider $github, CreateUserGitHubFollows $createUs ->limit(self::BatchSize) ->get(); + $deadline = now()->addSeconds(self::Budget); + foreach ($users as $user) { try { $createUserGitHubFollows->handle($user, notify: false); } catch (RequestException $exception) { - if (in_array($exception->response->status(), [403, 429], true)) { - $this->continueAt(now()->addMinutes(15)); + if ($this->isRateLimited($exception->response)) { + $retryAfter = $exception->response->header('Retry-After'); + $this->continueAt(is_numeric($retryAfter) ? now()->addSeconds((int) $retryAfter) : now()->addMinutes(15)); return; } @@ -74,6 +88,12 @@ public function handle(GitHubProvider $github, CreateUserGitHubFollows $createUs } $this->after = $user->id; + + if (now()->greaterThanOrEqualTo($deadline)) { + $this->continueAt(now()); + + return; + } } if ($users->count() < self::BatchSize) { @@ -85,6 +105,15 @@ public function handle(GitHubProvider $github, CreateUserGitHubFollows $createUs $this->continueAt(now()); } + /** + * GitHub answers a rate limit, primary or secondary, with a 429 or a 403 that says so. Any other 403 won't go away by + * waiting, so that user is skipped rather than holding up everyone after them. + */ + private function isRateLimited(Response $response): bool + { + return $response->status() === 429 || ($response->status() === 403 && str_contains(mb_strtolower($response->body()), 'rate limit')); + } + /** * Some queues can't hold a job back more than 15 minutes, so a longer wait reads the rate limit again by then. */ diff --git a/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php b/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php index ed9cf1b12..8d2d8570e 100644 --- a/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php +++ b/tests/Unit/Jobs/ImportAllGitHubFollowsJobTest.php @@ -79,7 +79,7 @@ 'reset in an hour' => [1000, 60, 15], ]); -it('waits 15 minutes when github rate limits it, and imports that user then', function (int $status): void { +it('waits when github rate limits it, as long as github asks or 15 minutes, and imports that user then', function (int $status, array $headers, int $delaySeconds): void { Queue::fake(); $this->freezeSecond(); @@ -89,24 +89,52 @@ $github->shouldReceive('rateLimit')->andReturnNull(); $github->shouldReceive('followingIds')->with($imported->handle, 10)->andReturn(['users' => [], 'organizations' => []]); $github->shouldReceive('followerIds')->andReturn([]); - $github->shouldReceive('followingIds')->with($limited->handle, 10)->andThrow(new RequestException(new Response(new Psr7Response($status)))); + $github->shouldReceive('followingIds')->with($limited->handle, 10)->andThrow(new RequestException(new Response(new Psr7Response($status, $headers, '{"message":"You have exceeded a secondary rate limit."}')))); new ImportAllGitHubFollowsJob()->handle($github, resolve(CreateUserGitHubFollows::class)); expect($limited->refresh()->github_follows_imported_at)->toBeNull(); Queue::assertPushed(ImportAllGitHubFollowsJob::class, fn (ImportAllGitHubFollowsJob $job): bool => $job->after === $imported->id - && CarbonImmutable::instance($job->delay)->equalTo(now()->addMinutes(15))); -})->with([403, 429]); + && CarbonImmutable::instance($job->delay)->equalTo(now()->addSeconds($delaySeconds))); +})->with([ + '403' => [403, [], 900], + '429' => [429, [], 900], + 'retry after' => [403, ['Retry-After' => '60'], 60], +]); + +it('leaves the rest of the batch to the next job once it has spent its budget', function (): void { + Queue::fake(); + $this->freezeSecond(); + + [$slow, $next] = User::factory()->count(2)->create()->sortBy('id')->values(); + + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('rateLimit')->andReturnNull(); + $github->shouldReceive('followingIds')->with($slow->handle, 10)->andReturnUsing(function (): array { + $this->travel(ImportAllGitHubFollowsJob::Budget)->seconds(); + + return ['users' => [], 'organizations' => []]; + }); + $github->shouldReceive('followerIds')->andReturn([]); + $github->shouldReceive('followingIds')->with($next->handle, 10)->never(); + + new ImportAllGitHubFollowsJob()->handle($github, resolve(CreateUserGitHubFollows::class)); + + expect($slow->refresh()->github_follows_imported_at)->not->toBeNull(); + Queue::assertPushed(ImportAllGitHubFollowsJob::class, fn (ImportAllGitHubFollowsJob $job): bool => $job->after === $slow->id + && CarbonImmutable::instance($job->delay)->equalTo(now())); +}); it('skips the users github fails on, reporting all but the accounts renamed since their last sign-in', function (): void { Queue::fake(); Exceptions::fake(); - [$renamed, $failing, $unreachable, $user] = User::factory()->count(4)->create()->sortBy('id')->values(); + [$renamed, $forbidden, $failing, $unreachable, $user] = User::factory()->count(5)->create()->sortBy('id')->values(); $github = $this->mock(GitHubProvider::class); $github->shouldReceive('rateLimit')->andReturnNull(); $github->shouldReceive('followingIds')->with($renamed->handle, 10)->andThrow(new RequestException(new Response(new Psr7Response(404)))); + $github->shouldReceive('followingIds')->with($forbidden->handle, 10)->andThrow(new RequestException(new Response(new Psr7Response(403, [], '{"message":"Forbidden"}')))); $github->shouldReceive('followingIds')->with($failing->handle, 10)->andThrow(new RequestException(new Response(new Psr7Response(500)))); $github->shouldReceive('followingIds')->with($unreachable->handle, 10)->andThrow(new ConnectionException()); $github->shouldReceive('followingIds')->with($user->handle, 10)->andReturn(['users' => [], 'organizations' => []]); @@ -117,5 +145,5 @@ expect($renamed->refresh()->github_follows_imported_at)->toBeNull() ->and($user->refresh()->github_follows_imported_at)->not->toBeNull(); - Exceptions::assertReportedCount(2); + Exceptions::assertReportedCount(3); });