From c0d0d578f9cd0015d7db5d4f5e06ddab8832de5c Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 14:56:10 -0300 Subject: [PATCH 01/19] Stop paying X for token checks the refresh already proves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RefreshSocialToken verified rotating-refresh platforms instead of refreshing them. On a still-valid token verify() only called the platform's verify endpoint and left token_expires_at untouched, so the account stayed inside RefreshExpiringTokens' 30-minute window and was re-read every 15 minutes until the token actually died. On X that endpoint is GET /2/users/me, billed as a "User: Read" ($0.010 per resource under X's pay-per-usage pricing). Simulating 24h of the scheduler against one connected X account: 36 billed reads per day, 24 of which renewed nothing, plus 180 minutes per day sitting on an expired token between expiry and the next tick. Refresh the token outright instead. A provider that hands back a fresh token has already confirmed the credential — it rejects a revoked one with a 4xx, which TokenRefreshClient maps to TokenExpiredException — so the verify call adds cost and nothing else. Record the confirmation in last_verified_at, and let the daily sweep trust it for 12 hours the way VerifyUpcomingPostConnections already does, so it stops re-reading accounts a refresh just proved valid. Same simulation after the change: 0 billed reads, 0 minutes expired. Two existing tests asserted the old policy (verify-first, refresh_token left unrotated) and now assert the new one. The rotation test still guards what made that policy attractive: a proactive rotation must not trip a false-positive disconnect. --- app/Jobs/RefreshSocialToken.php | 20 +++--- app/Jobs/VerifyWorkspaceConnections.php | 23 ++++++ app/Services/Social/ConnectionVerifier.php | 8 +++ tests/Feature/Jobs/RefreshSocialTokenTest.php | 70 +++++++++++++++---- .../VerifyWorkspaceConnectionsTest.php | 60 ++++++++++++++++ 5 files changed, 160 insertions(+), 21 deletions(-) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index c3678c96a..efb8c7ece 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -21,17 +21,21 @@ class RefreshSocialToken implements ShouldQueue public function __construct(public SocialAccount $account) {} + /** + * Refresh the token outright rather than verifying it first. + * + * A successful refresh already proves the credential is alive — the + * provider rejects a revoked one with a 4xx — so the verify endpoint adds + * nothing but cost. On X that endpoint is `GET /2/users/me`, billed as a + * "User: Read", and verifying a still-valid token left `token_expires_at` + * untouched: the account stayed inside RefreshExpiringTokens' window and + * was re-read every 15 minutes until the token actually died, which also + * left it expired for the stretch between expiry and the next tick. + */ public function handle(ConnectionVerifier $verifier): void { try { - if ($this->account->platform->extendsAccessTokenOnRefresh()) { - // Instagram/Threads extend the long-lived token itself and - // can't be refreshed once expired, so extend it while it's - // still valid instead of waiting for it to fail. - $verifier->refreshToken($this->account); - } else { - $verifier->verify($this->account); - } + $verifier->refreshToken($this->account); } catch (PlatformUnavailableException $e) { Log::warning('Token refresh skipped: platform unavailable', [ 'account_id' => $this->account->id, diff --git a/app/Jobs/VerifyWorkspaceConnections.php b/app/Jobs/VerifyWorkspaceConnections.php index 128965d95..0757f946a 100644 --- a/app/Jobs/VerifyWorkspaceConnections.php +++ b/app/Jobs/VerifyWorkspaceConnections.php @@ -26,6 +26,13 @@ class VerifyWorkspaceConnections implements ShouldQueue public int $timeout = 120; + // How long a recorded verification (SocialAccount::last_verified_at) is + // trusted before this sweep re-checks the account. RefreshSocialToken + // stamps that field on every successful token refresh, and a refresh + // proves the credential just as well as the verify endpoint does — without + // the per-call charge providers like X bill for reading a profile. + private const VERIFIED_WITHIN_HOURS = 12; + public function __construct(public Workspace $workspace) {} public function handle(ConnectionVerifier $verifier): void @@ -42,6 +49,10 @@ public function handle(ConnectionVerifier $verifier): void $disconnectedAccounts = collect(); foreach ($accounts as $account) { + if ($this->recentlyProvenValid($account)) { + continue; + } + if ($this->verifyAccount($verifier, $account)) { // If was TokenExpired but now verified OK, mark as connected again if ($account->status === Status::TokenExpired) { @@ -59,6 +70,18 @@ public function handle(ConnectionVerifier $verifier): void } } + /** + * Only a Connected account can be skipped. A TokenExpired one still needs + * the call: verifying it is how it gets promoted back to Connected, so + * trusting a stale stamp would strand a recovered account forever. + */ + private function recentlyProvenValid(SocialAccount $account): bool + { + return $account->status === Status::Connected + && $account->last_verified_at !== null + && $account->last_verified_at->isAfter(now()->subHours(self::VERIFIED_WITHIN_HOURS)); + } + private function verifyAccount(ConnectionVerifier $verifier, SocialAccount $account): bool { try { diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index 954e83298..cb08da12f 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -152,6 +152,14 @@ public function refreshToken(SocialAccount $account): void // Mastodon tokens don't expire either. default => null, }; + + // A provider that hands back a fresh token has just confirmed the + // credential, so record it as a verification: jobs that would + // otherwise call the (often billed) verify endpoint can trust this + // instead. Platforms with nothing to refresh prove nothing here. + if ($account->platform->hasTokenRefreshFlow()) { + $account->update(['last_verified_at' => now()]); + } } finally { $lock->release(); } diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 898950c30..35caff57f 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -26,23 +26,22 @@ ]); }); -test('refresh job routes through verify (access-token-first) not refreshToken', function () { +test('refresh job routes through refreshToken, never the billed verify endpoint', function () { $verifier = mock(ConnectionVerifier::class); - $verifier->shouldReceive('verify')->once()->with( + $verifier->shouldReceive('refreshToken')->once()->with( Mockery::on(fn ($account) => $account->id === $this->account->id) ); - $verifier->shouldNotReceive('refreshToken'); + $verifier->shouldNotReceive('verify'); app()->instance(ConnectionVerifier::class, $verifier); (new RefreshSocialToken($this->account))->handle($verifier); }); -test('proactive refresh does NOT rotate the X refresh token while the access token still works', function () { +test('proactive refresh rotates the X refresh token without disconnecting the account', function () { Http::fake([ - config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ - 'access_token' => 'should-not-be-used', - 'refresh_token' => 'should-not-be-used', + 'access_token' => 'rotated-access-token', + 'refresh_token' => 'rotated-refresh-token', 'expires_in' => 7200, ], 200), ]); @@ -55,9 +54,9 @@ (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); - Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me')); - Http::assertNotSent(fn ($request) => str_contains($request->url(), '/oauth2/token')); - expect($this->account->fresh()->refresh_token)->toBe('original-refresh-token'); + // X single-uses the refresh_token, so rotating one proactively has to leave + // the account healthy instead of tripping a false-positive disconnect. + expect($this->account->fresh()->refresh_token)->toBe('rotated-refresh-token'); expect($this->account->fresh()->status)->toBe(Status::Connected); }); @@ -133,7 +132,7 @@ Queue::fake(); $verifier = mock(ConnectionVerifier::class); - $verifier->shouldReceive('verify')->once()->andThrow( + $verifier->shouldReceive('refreshToken')->once()->andThrow( new TokenExpiredException('refresh_token revoked') ); app()->instance(ConnectionVerifier::class, $verifier); @@ -155,7 +154,7 @@ }); $verifier = mock(ConnectionVerifier::class); - $verifier->shouldReceive('verify')->once()->andThrow(new RuntimeException('network blip')); + $verifier->shouldReceive('refreshToken')->once()->andThrow(new RuntimeException('network blip')); app()->instance(ConnectionVerifier::class, $verifier); (new RefreshSocialToken($this->account))->handle($verifier); @@ -173,7 +172,7 @@ }); $verifier = mock(ConnectionVerifier::class); - $verifier->shouldReceive('verify')->once()->andThrow( + $verifier->shouldReceive('refreshToken')->once()->andThrow( new PlatformUnavailableException('X API returned 503 during token refresh', 503) ); app()->instance(ConnectionVerifier::class, $verifier); @@ -183,3 +182,48 @@ expect($this->account->fresh()->status)->toBe(Status::Connected); Queue::assertNotPushed(SendNotification::class); }); + +test('proactive refresh renews a still-valid X token without spending a billed user read', function () { + Http::fake([ + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'access_token' => 'rotated-access-token', + 'refresh_token' => 'rotated-refresh-token', + 'expires_in' => 7200, + ], 200), + ]); + + $this->account->update([ + 'access_token' => 'original-access-token', + 'token_expires_at' => now()->addMinutes(20), + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + // GET /2/users/me is a billed "User: Read" ($0.010). A successful token + // refresh already proves the credential works, so it must not be called. + Http::assertNotSent(fn ($request) => str_contains($request->url(), '/users/me')); + Http::assertSent(fn ($request) => str_contains($request->url(), '/oauth2/token')); + + expect($this->account->fresh()->access_token)->toBe('rotated-access-token'); + expect($this->account->fresh()->token_expires_at->isAfter(now()->addHour()))->toBeTrue(); +}); + +test('a successful refresh stamps last_verified_at so other jobs can skip verifying', function () { + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'access_token' => 'rotated-access-token', + 'refresh_token' => 'rotated-refresh-token', + 'expires_in' => 7200, + ], 200), + ]); + + $this->account->update([ + 'token_expires_at' => now()->addMinutes(20), + 'last_verified_at' => null, + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + expect($this->account->fresh()->last_verified_at)->not->toBeNull(); +}); diff --git a/tests/Feature/VerifyWorkspaceConnectionsTest.php b/tests/Feature/VerifyWorkspaceConnectionsTest.php index 199767010..827e70212 100644 --- a/tests/Feature/VerifyWorkspaceConnectionsTest.php +++ b/tests/Feature/VerifyWorkspaceConnectionsTest.php @@ -10,6 +10,7 @@ use App\Models\SocialAccount; use App\Models\Workspace; use App\Services\Social\ConnectionVerifier; +use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Mail; test('job does nothing when workspace has no connected accounts', function () { @@ -156,3 +157,62 @@ Mail::assertNothingSent(); }); + +test('daily sweep skips verifying a connected account a recent refresh already proved valid', function () { + Mail::fake(); + Http::fake([ + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $workspace = Workspace::factory()->create(); + SocialAccount::factory()->x()->create([ + 'workspace_id' => $workspace->id, + 'status' => Status::Connected, + 'last_verified_at' => now()->subHours(2), + ]); + + VerifyWorkspaceConnections::dispatch($workspace); + + // A successful token refresh within the trust window already proved the + // credential — re-reading the profile would only burn a billed User Read. + Http::assertNotSent(fn ($request) => str_contains($request->url(), '/users/me')); + Mail::assertNothingSent(); +}); + +test('daily sweep still verifies a connected account whose last_verified_at is stale', function () { + Mail::fake(); + Http::fake([ + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $workspace = Workspace::factory()->create(); + SocialAccount::factory()->x()->create([ + 'workspace_id' => $workspace->id, + 'status' => Status::Connected, + 'last_verified_at' => now()->subHours(20), + ]); + + VerifyWorkspaceConnections::dispatch($workspace); + + Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me')); +}); + +test('daily sweep still verifies a TokenExpired account despite a fresh last_verified_at', function () { + Mail::fake(); + Http::fake([ + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $workspace = Workspace::factory()->create(); + $account = SocialAccount::factory()->x()->create([ + 'workspace_id' => $workspace->id, + 'status' => Status::TokenExpired, + 'last_verified_at' => now()->subMinutes(5), + ]); + + VerifyWorkspaceConnections::dispatch($workspace); + + // Skipping here would strand a recovered account in TokenExpired forever. + Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me')); + expect($account->fresh()->status)->toBe(Status::Connected); +}); From 97755784eba8f7ce5fb4f98f3adb3c399626ac76 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 15:06:25 -0300 Subject: [PATCH 02/19] Don't disconnect an account whose access token still works MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refreshing instead of verifying removed a safety net the old verify-first path had: if the refresh is rejected, the account was marked TokenExpired outright. But a rejected refresh does not mean the connection is dead. X and LinkedIn single-use their refresh_token, so a token a concurrent refresh already consumed comes back 4xx while the current access_token keeps working. An account with no refresh_token at all fails even earlier, without a single call being made. Verified against main: both cases stayed Connected before, and became TokenExpired after. PublishToSocialPlatform hard-fails every post for a TokenExpired account, so this killed posts the access_token would have published, up to 30 minutes before the token was actually due to expire — and emailed the owner a disconnect notice for it. Fall back to verifying the access token before disconnecting. This is the only path in the job that reaches the billed verify endpoint, and only after a refresh has already been rejected, so the healthy path stays at zero reads (re-confirmed: 0 reads and 0 expired minutes across a simulated 24h). A failure that can't be attributed to the token — platform down, network blip — leaves the account alone instead of disconnecting it on noise. Also covers two gaps found while reviewing: a lock-skipped refresh must not record a verification it never performed, and a platform with nothing to refresh must not be recorded as verified either. Both already behaved correctly; they now have tests so the daily sweep can't start trusting a stamp nobody earned. --- app/Jobs/RefreshSocialToken.php | 35 ++++++ tests/Feature/Jobs/RefreshSocialTokenTest.php | 100 ++++++++++++++++++ 2 files changed, 135 insertions(+) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index efb8c7ece..6d1b9ac7a 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -43,6 +43,10 @@ public function handle(ConnectionVerifier $verifier): void 'error' => $e->getMessage(), ]); } catch (TokenExpiredException $e) { + if ($this->accessTokenStillWorks($verifier)) { + return; + } + $this->account->markAsTokenExpired($e->getMessage()); } catch (Throwable $e) { Log::warning('Proactive token refresh failed', [ @@ -52,4 +56,35 @@ public function handle(ConnectionVerifier $verifier): void ]); } } + + /** + * A rejected refresh does not on its own mean the connection is dead. + * Providers that single-use their refresh_token (X, LinkedIn) reject one a + * concurrent refresh already consumed while the current access_token keeps + * working, and an account with no refresh_token at all fails here without + * any call being made. PublishToSocialPlatform hard-fails every post for a + * TokenExpired account, so disconnecting on a refresh rejection alone kills + * posts the access_token would still have published. + * + * This is the only place the (often billed) verify endpoint is reached from + * this job, and only after a refresh has already been rejected. A failure + * we can't attribute to the token — the platform being down, a network + * blip — leaves the account alone rather than disconnecting it on noise. + */ + private function accessTokenStillWorks(ConnectionVerifier $verifier): bool + { + try { + return $verifier->verify($this->account); + } catch (TokenExpiredException) { + return false; + } catch (Throwable $e) { + Log::warning('Access token fallback check failed after a rejected refresh', [ + 'account_id' => $this->account->id, + 'platform' => $this->account->platform->value, + 'error' => $e->getMessage(), + ]); + + return true; + } + } } diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 35caff57f..7b460b848 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -12,6 +12,7 @@ use App\Models\User; use App\Models\Workspace; use App\Services\Social\ConnectionVerifier; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Queue; @@ -135,6 +136,10 @@ $verifier->shouldReceive('refreshToken')->once()->andThrow( new TokenExpiredException('refresh_token revoked') ); + // The access_token is dead too, so there is nothing left to fall back to. + $verifier->shouldReceive('verify')->once()->andThrow( + new TokenExpiredException('X access token is invalid or expired') + ); app()->instance(ConnectionVerifier::class, $verifier); (new RefreshSocialToken($this->account))->handle($verifier); @@ -227,3 +232,98 @@ expect($this->account->fresh()->last_verified_at)->not->toBeNull(); }); + +test('a rejected refresh does not disconnect an account whose access token still works', function () { + Queue::fake(); + + Http::fake([ + // X single-uses the refresh_token; a concurrent refresh already burned + // this one, so the provider rejects it — but the access_token is alive. + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'error' => 'invalid_grant', + 'error_description' => 'Value passed for the token was invalid.', + ], 400), + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $this->account->update([ + 'access_token' => 'still-valid-access-token', + 'refresh_token' => 'already-consumed-by-a-race', + 'token_expires_at' => now()->addMinutes(20), + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + // PublishToSocialPlatform hard-fails posts for a TokenExpired account, so + // disconnecting here would kill posts the access_token could still publish. + expect($this->account->fresh()->status)->toBe(Status::Connected); + Queue::assertNotPushed(SendNotification::class); +}); + +test('an account with no refresh token stays connected while its access token works', function () { + Queue::fake(); + + Http::fake([ + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $this->account->update([ + 'access_token' => 'still-valid-access-token', + 'refresh_token' => null, + 'token_expires_at' => now()->addMinutes(20), + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + expect($this->account->fresh()->status)->toBe(Status::Connected); + Queue::assertNotPushed(SendNotification::class); +}); + +test('a rejected refresh DOES disconnect once the access token is dead too', function () { + Queue::fake(); + + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'error' => 'invalid_grant', + 'error_description' => 'refresh_token revoked', + ], 400), + config('trypost.platforms.x.api').'/users/me' => Http::response([ + 'title' => 'Unauthorized', + 'status' => 401, + ], 401), + ]); + + $this->account->update([ + 'access_token' => 'dead-access-token', + 'refresh_token' => 'revoked-refresh-token', + 'token_expires_at' => now()->addMinutes(20), + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + expect($this->account->fresh()->status)->toBe(Status::TokenExpired); +}); + +test('lock skipped by a concurrent refresh does not record a verification', function () { + Cache::lock("token_refresh:{$this->account->id}", 30)->get(); + + $this->account->update(['last_verified_at' => null]); + + app(ConnectionVerifier::class)->refreshToken($this->account); + + // Nothing was refreshed here, so nothing was proven — stamping would let + // the daily sweep skip an account no one actually checked. + expect($this->account->fresh()->last_verified_at)->toBeNull(); +}); + +test('a platform with nothing to refresh is never recorded as verified', function () { + $account = SocialAccount::factory()->mastodon()->create([ + 'workspace_id' => $this->workspace->id, + 'status' => Status::Connected, + 'last_verified_at' => null, + ]); + + app(ConnectionVerifier::class)->refreshToken($account); + + expect($account->fresh()->last_verified_at)->toBeNull(); +}); From 4d2795a8ef7d6af829267a009cb3045d3e8b4709 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 15:20:02 -0300 Subject: [PATCH 03/19] Only record a verification something actually proved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous two commits turned up four issues, all in code they introduced. The stamp lived inside ConnectionVerifier::refreshToken(), which refreshThenVerify() also calls — there the refresh can succeed and the verify that follows still fail. The stamp was already written by then, vouching for a credential nothing confirmed. Normally harmless, because the caller marks the account TokenExpired and recentlyProvenValid() only skips Connected ones, but markAsTokenExpired() silently no-ops when its status lock is held by a concurrent publish. The account then stays Connected with a fresh stamp, and both skip-windows wave it through: 40 minutes before publishing, 12 hours in the daily sweep. Move the stamp to the caller that owns the outcome. TokenRefreshClient classifies on HTTP status alone and never inspects the body, so a 200 carrying an empty access_token is stored as-is and was then recorded as healthy. (A missing key rather than an empty one can't get that far — access_token is NOT NULL, so the write throws first.) Guard on a filled token. The fallback added in 9775578 called verify(), which for an already-expired account — RefreshExpiringTokens selects those too — runs refreshThenVerify() and re-sends the refresh_token the provider just rejected, and on Bluesky re-runs the password re-auth AT Proto rate-limits per account. Its own docblock claimed it only reached the verify endpoint. Add verifyAccessToken(), which checks the stored token and nothing else. VerifyWorkspaceConnections read last_verified_at without ever writing it, so an account it had just confirmed healthy still burned a fresh call minutes later when a post entered the risk window. Stamp on its success path too. Also corrects the RefreshExpiringTokens docblock, which still described the verify-first behaviour removed in c0d0d57. Re-ran the 24h whole-scheduler simulation: still 0 billed reads, 0 expired minutes, account Connected at the end. --- .../Commands/RefreshExpiringTokens.php | 8 +- app/Jobs/RefreshSocialToken.php | 28 ++++++- app/Jobs/VerifyWorkspaceConnections.php | 1 + app/Services/Social/ConnectionVerifier.php | 25 ++++-- tests/Feature/Jobs/RefreshSocialTokenTest.php | 76 ++++++++++++++++++- .../VerifyWorkspaceConnectionsTest.php | 20 +++++ 6 files changed, 144 insertions(+), 14 deletions(-) diff --git a/app/Console/Commands/RefreshExpiringTokens.php b/app/Console/Commands/RefreshExpiringTokens.php index 9dc08b661..b6906ac07 100644 --- a/app/Console/Commands/RefreshExpiringTokens.php +++ b/app/Console/Commands/RefreshExpiringTokens.php @@ -18,10 +18,10 @@ class RefreshExpiringTokens extends Command protected $description = 'Proactively refresh social tokens before they expire'; /** - * Rotating refresh_token platforms only need a short lead: verify() won't - * rotate a still-valid token, so we catch them right before or after expiry. - * Extension-model platforms (Instagram/Threads) can't be refreshed once - * expired, so they get a much wider lead to survive queue backlog. + * Rotating refresh_token platforms get a short lead: RefreshSocialToken + * rotates on every run, so a wider window would only rotate more often for + * no gain. Extension-model platforms (Instagram/Threads) can't be refreshed + * once expired, so they get a much wider lead to survive queue backlog. */ public function handle(): void { diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 6d1b9ac7a..3ea76b866 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -36,6 +36,7 @@ public function handle(ConnectionVerifier $verifier): void { try { $verifier->refreshToken($this->account); + $this->recordVerification(); } catch (PlatformUnavailableException $e) { Log::warning('Token refresh skipped: platform unavailable', [ 'account_id' => $this->account->id, @@ -74,7 +75,7 @@ public function handle(ConnectionVerifier $verifier): void private function accessTokenStillWorks(ConnectionVerifier $verifier): bool { try { - return $verifier->verify($this->account); + return $verifier->verifyAccessToken($this->account); } catch (TokenExpiredException) { return false; } catch (Throwable $e) { @@ -87,4 +88,29 @@ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool return true; } } + + /** + * Record the refresh as a verification, so the daily sweep and the + * pre-publish check can skip their own (often billed) verify call. + * + * Only a refresh that actually happened proves anything, and only one that + * came back with a usable token: TokenRefreshClient classifies on HTTP + * status alone and never inspects the body, so a 200 carrying an empty + * token would otherwise be recorded as healthy. This is deliberately not + * done inside ConnectionVerifier::refreshToken() — refreshThenVerify() + * calls it and can still fail on the verify that follows, and a stamp + * written there would vouch for a credential nothing ever confirmed. + */ + private function recordVerification(): void + { + if (! $this->account->platform->hasTokenRefreshFlow()) { + return; + } + + if (blank($this->account->access_token)) { + return; + } + + $this->account->update(['last_verified_at' => now()]); + } } diff --git a/app/Jobs/VerifyWorkspaceConnections.php b/app/Jobs/VerifyWorkspaceConnections.php index 0757f946a..815c367e1 100644 --- a/app/Jobs/VerifyWorkspaceConnections.php +++ b/app/Jobs/VerifyWorkspaceConnections.php @@ -86,6 +86,7 @@ private function verifyAccount(ConnectionVerifier $verifier, SocialAccount $acco { try { $verifier->verify($account); + $account->update(['last_verified_at' => now()]); return true; } catch (PlatformUnavailableException $e) { diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index cb08da12f..84b458db6 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -96,6 +96,23 @@ private function refreshThenVerify(SocialAccount $account, ?TokenExpiredExceptio } } + /** + * Check the stored access token exactly as it is, skipping the + * refresh-and-retry ladder verify() runs. + * + * Callers that have just had a refresh rejected need this: routing through + * verify() would re-send the refresh_token the provider only just rejected, + * and on Bluesky re-run the password re-auth AT Proto rate-limits per + * account. + * + * @throws TokenExpiredException if the access token itself is rejected + * @throws PlatformUnavailableException if the platform is unreachable + */ + public function verifyAccessToken(SocialAccount $account): bool + { + return $this->callVerifyEndpoint($account); + } + /** * @throws TokenExpiredException */ @@ -152,14 +169,6 @@ public function refreshToken(SocialAccount $account): void // Mastodon tokens don't expire either. default => null, }; - - // A provider that hands back a fresh token has just confirmed the - // credential, so record it as a verification: jobs that would - // otherwise call the (often billed) verify endpoint can trust this - // instead. Platforms with nothing to refresh prove nothing here. - if ($account->platform->hasTokenRefreshFlow()) { - $account->update(['last_verified_at' => now()]); - } } finally { $lock->release(); } diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 7b460b848..ad3ab81ba 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -137,7 +137,7 @@ new TokenExpiredException('refresh_token revoked') ); // The access_token is dead too, so there is nothing left to fall back to. - $verifier->shouldReceive('verify')->once()->andThrow( + $verifier->shouldReceive('verifyAccessToken')->once()->andThrow( new TokenExpiredException('X access token is invalid or expired') ); app()->instance(ConnectionVerifier::class, $verifier); @@ -327,3 +327,77 @@ expect($account->fresh()->last_verified_at)->toBeNull(); }); + +test('a refresh whose follow-up verify fails is not recorded as a verification', function () { + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'access_token' => 'fresh-but-rejected', + 'refresh_token' => 'rt-new', + 'expires_in' => 7200, + ], 200), + config('trypost.platforms.x.api').'/users/me' => Http::response(['title' => 'Unauthorized'], 401), + ]); + + $this->account->update([ + 'token_expires_at' => now()->subMinute(), + 'last_verified_at' => null, + ]); + + try { + app(ConnectionVerifier::class)->verify($this->account); + } catch (TokenExpiredException) { + // expected — the refreshed token is rejected too + } + + // The refresh succeeded but the credential was never proven good. Stamping + // here lets both skip-windows wave through an account nobody verified. + expect($this->account->fresh()->last_verified_at)->toBeNull(); +}); + +test('a refresh that returns an empty access token is not recorded as a verification', function () { + // TokenRefreshClient classifies on HTTP status alone and never inspects the + // body, so a 200 carrying an empty token is stored as-is. + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'access_token' => '', + 'refresh_token' => 'rt-new', + 'expires_in' => 7200, + ], 200), + ]); + + $this->account->update([ + 'token_expires_at' => now()->addMinutes(20), + 'last_verified_at' => null, + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + expect($this->account->fresh()->last_verified_at)->toBeNull(); +}); + +test('a rejected refresh is not re-sent before the access token is checked', function () { + Queue::fake(); + + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'error' => 'invalid_grant', + ], 400), + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $this->account->update([ + 'access_token' => 'still-valid-access-token', + 'refresh_token' => 'already-consumed-by-a-race', + 'token_expires_at' => now()->subMinute(), + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + // Going through verify() would re-send the refresh_token the provider just + // rejected — and on Bluesky re-run the rate-limited password re-auth. + $refreshCalls = collect(Http::recorded()) + ->filter(fn ($pair) => str_contains($pair[0]->url(), '/oauth2/token')) + ->count(); + + expect($refreshCalls)->toBe(1); +}); diff --git a/tests/Feature/VerifyWorkspaceConnectionsTest.php b/tests/Feature/VerifyWorkspaceConnectionsTest.php index 827e70212..685e8a115 100644 --- a/tests/Feature/VerifyWorkspaceConnectionsTest.php +++ b/tests/Feature/VerifyWorkspaceConnectionsTest.php @@ -216,3 +216,23 @@ Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me')); expect($account->fresh()->status)->toBe(Status::Connected); }); + +test('daily sweep records its own successful verification', function () { + Mail::fake(); + Http::fake([ + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $workspace = Workspace::factory()->create(); + $account = SocialAccount::factory()->x()->create([ + 'workspace_id' => $workspace->id, + 'status' => Status::Connected, + 'last_verified_at' => null, + ]); + + VerifyWorkspaceConnections::dispatch($workspace); + + // Otherwise VerifyUpcomingPostConnections burns a fresh call minutes later + // on an account this sweep just confirmed healthy. + expect($account->fresh()->last_verified_at)->not->toBeNull(); +}); From 48e419539b10a53d0774e57742cc09b28785eb3f Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 15:31:50 -0300 Subject: [PATCH 04/19] Fall back to the token a concurrent refresh persisted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fallback added in 9775578 judged the in-memory access_token, which is exactly the one that is stale when a refresh loses a race. X single-uses the refresh_token, so when two refreshes overlap the loser's token comes back 400 invalid_grant. Its in-memory instance still holds the pair the winner has already rotated away, so verifying it 401s and the account is marked TokenExpired — while the row in the database holds a perfectly healthy token the winner just wrote. Verified against main: a concurrent rotation leaves the account Connected there and TokenExpired here. refreshThenVerify() already handles this by reloading and retrying with whatever was persisted; the new path skipped that because it never went through refreshThenVerify. Reload before judging. This is the failure path only, so the healthy path is untouched — the whole-scheduler simulation still reports 0 billed reads, 0 expired minutes, Connected at the end. --- app/Jobs/RefreshSocialToken.php | 6 ++++ tests/Feature/Jobs/RefreshSocialTokenTest.php | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 3ea76b866..2471235ec 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -74,6 +74,12 @@ public function handle(ConnectionVerifier $verifier): void */ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool { + // A concurrent refresh may have persisted a new pair while ours was in + // flight, which is why ours was rejected. This instance still holds the + // token that was rotated away, so reload before judging it — otherwise + // the winner's healthy account gets disconnected. + $this->account->refresh(); + try { return $verifier->verifyAccessToken($this->account); } catch (TokenExpiredException) { diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index ad3ab81ba..163d489bf 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -13,6 +13,7 @@ use App\Models\Workspace; use App\Services\Social\ConnectionVerifier; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Queue; @@ -401,3 +402,38 @@ expect($refreshCalls)->toBe(1); }); + +test('a refresh lost to a concurrent one falls back to the token that won', function () { + Queue::fake(); + + $api = config('trypost.platforms.x.api'); + Http::fake([ + // Our refresh_token was already consumed by the process that won. + $api.'/oauth2/token' => Http::response(['error' => 'invalid_grant'], 400), + $api.'/users/me' => function ($request) { + $auth = $request->header('Authorization')[0] ?? ''; + + return str_contains($auth, 'winner-access-token') + ? Http::response(['data' => ['id' => '123']], 200) + : Http::response(['title' => 'Unauthorized', 'status' => 401], 401); + }, + ]); + + $this->account->update([ + 'access_token' => 'stale-access-token', + 'refresh_token' => 'stale-refresh-token', + 'token_expires_at' => now()->addMinutes(20), + ]); + + // The winner persisted its new pair while ours was in flight; this + // instance still holds the rotated-away one. + DB::table('social_accounts')->where('id', $this->account->id)->update([ + 'access_token' => 'winner-access-token', + 'refresh_token' => 'winner-refresh-token', + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + expect($this->account->fresh()->status)->toBe(Status::Connected); + Queue::assertNotPushed(SendNotification::class); +}); From 8a3cfe83a218178beab0c767177c6cc429cb4c87 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 15:37:00 -0300 Subject: [PATCH 05/19] Don't record a verification when the lock skipped the refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refreshToken() returns normally — no exception — when another process already holds the per-account lock, so the caller can't tell "refreshed" from "did nothing". Moving the stamp out of the verifier in 4d2795a lost that distinction: RefreshSocialToken stamped last_verified_at on a run that made zero HTTP calls. The account is then vouched for by nobody: the daily sweep skips it for 12 hours and the pre-publish check for 40 minutes. If the concurrent refresh also failed, nothing ever confirmed the credential. Have refreshToken() report whether it actually ran. Callers that ignore the return value are unaffected. The test meant to cover this called refreshToken() directly rather than going through the job, so it kept passing while the job path was broken — it now exercises the job and asserts no HTTP call was made. --- app/Jobs/RefreshSocialToken.php | 8 ++++---- app/Services/Social/ConnectionVerifier.php | 10 ++++++++-- tests/Feature/Jobs/RefreshSocialTokenTest.php | 12 +++++++++--- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 2471235ec..fd0f0e057 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -35,8 +35,9 @@ public function __construct(public SocialAccount $account) {} public function handle(ConnectionVerifier $verifier): void { try { - $verifier->refreshToken($this->account); - $this->recordVerification(); + if ($verifier->refreshToken($this->account)) { + $this->recordVerification(); + } } catch (PlatformUnavailableException $e) { Log::warning('Token refresh skipped: platform unavailable', [ 'account_id' => $this->account->id, @@ -99,8 +100,7 @@ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool * Record the refresh as a verification, so the daily sweep and the * pre-publish check can skip their own (often billed) verify call. * - * Only a refresh that actually happened proves anything, and only one that - * came back with a usable token: TokenRefreshClient classifies on HTTP + * Only a refresh that came back with a usable token proves anything: TokenRefreshClient classifies on HTTP * status alone and never inspects the body, so a 200 carrying an empty * token would otherwise be recorded as healthy. This is deliberately not * done inside ConnectionVerifier::refreshToken() — refreshThenVerify() diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index 84b458db6..5e80dd3a0 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -141,10 +141,14 @@ private function callVerifyEndpoint(SocialAccount $account): bool * use verify() instead. This method always attempts a refresh under * the per-account lock. * + * @return bool whether a refresh actually ran. False means another process + * already held the lock and this call did nothing, so callers + * must not treat it as having proven anything. + * * @throws TokenExpiredException if refresh is rejected by the provider (4xx) * @throws PlatformUnavailableException if the platform is unreachable (5xx / network) */ - public function refreshToken(SocialAccount $account): void + public function refreshToken(SocialAccount $account): bool { $lock = Cache::lock("token_refresh:{$account->id}", 30); @@ -152,7 +156,7 @@ public function refreshToken(SocialAccount $account): void // Another process is already refreshing this token $account->refresh(); - return; + return false; } try { @@ -169,6 +173,8 @@ public function refreshToken(SocialAccount $account): void // Mastodon tokens don't expire either. default => null, }; + + return true; } finally { $lock->release(); } diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 163d489bf..65283ad86 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -306,14 +306,20 @@ }); test('lock skipped by a concurrent refresh does not record a verification', function () { - Cache::lock("token_refresh:{$this->account->id}", 30)->get(); + Http::fake([config('trypost.platforms.x.api').'/*' => Http::response([], 200)]); + + $this->account->update([ + 'last_verified_at' => null, + 'token_expires_at' => now()->addMinutes(20), + ]); - $this->account->update(['last_verified_at' => null]); + Cache::lock("token_refresh:{$this->account->id}", 30)->get(); - app(ConnectionVerifier::class)->refreshToken($this->account); + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); // Nothing was refreshed here, so nothing was proven — stamping would let // the daily sweep skip an account no one actually checked. + Http::assertNothingSent(); expect($this->account->fresh()->last_verified_at)->toBeNull(); }); From 5001c9a9600200ce89cd3648d0ea954ef3c5c520 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 15:46:55 -0300 Subject: [PATCH 06/19] Close the two concurrency gaps the refresh path leaves open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are pre-existing, but this branch raises the exposure to them from 12 to 16 refreshes per account per day. The per-account lock lasted 30 seconds, exactly the HTTP client's default read timeout — so a refresh could outlive the lock that protects it. Bluesky is the worst case, refreshing with two sequential calls (refreshSession, then the createSession re-auth), each bounded by connect + read timeouts: up to ~80 seconds under one 30-second lock. Once it lapses, a second process refreshes with the same single-use refresh_token and one of the two is rejected. Name the TTL, set it past the ceiling, and write down the invariant so a future slower refresh doesn't quietly break it. RefreshSocialToken was not unique. RefreshExpiringTokens re-selects an account until token_expires_at moves, and that only happens once the job runs — so a queue more than one tick behind stacked a job per tick for the same account, each rotating a single-use refresh_token again for nothing and widening the gap where a worker death loses the pair. Key it by account like VerifyUpcomingPostConnections already does. Cadence is unchanged: the whole-scheduler simulation still reports 16 refreshes, 0 billed reads and 0 expired minutes over 24h. --- app/Jobs/RefreshSocialToken.php | 15 ++++++++++++++- app/Services/Social/ConnectionVerifier.php | 18 +++++++++++++++++- .../Commands/RefreshExpiringTokensTest.php | 19 +++++++++++++++++++ tests/Feature/Jobs/RefreshSocialTokenTest.php | 10 ++++++++++ 4 files changed, 60 insertions(+), 2 deletions(-) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index fd0f0e057..348fd8535 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -8,19 +8,32 @@ use App\Exceptions\TokenExpiredException; use App\Models\SocialAccount; use App\Services\Social\ConnectionVerifier; +use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; use Illuminate\Support\Facades\Log; use Throwable; -class RefreshSocialToken implements ShouldQueue +class RefreshSocialToken implements ShouldBeUnique, ShouldQueue { use Queueable; public int $tries = 1; + // Covers the full schedule cadence. RefreshExpiringTokens re-selects an + // account until its token_expires_at moves, which only happens once this + // job runs — so a backlogged queue would otherwise stack a job per tick, + // each rotating a single-use refresh_token again for nothing and widening + // the window where a worker death loses the pair. + public int $uniqueFor = 900; + public function __construct(public SocialAccount $account) {} + public function uniqueId(): string + { + return $this->account->id; + } + /** * Refresh the token outright rather than verifying it first. * diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index 5e80dd3a0..a397b3d21 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -25,6 +25,22 @@ class ConnectionVerifier { + /** + * How long the per-account refresh lock survives without being released. + * + * It has to outlast the slowest refresh a provider can make us wait for, + * or the lock lapses mid-flight and a second process refreshes with the + * same single-use refresh_token — leaving one of the two rejected. The + * ceiling is Bluesky, which refreshes with two sequential calls + * (refreshSession, then the createSession re-auth), each bounded by the + * HTTP client's connect and read timeouts. + * + * The lock is released in a finally block, so this only governs how long a + * worker that died mid-refresh blocks the next attempt — and the next + * scheduler tick is 15 minutes out either way. + */ + public const REFRESH_LOCK_SECONDS = 120; + /** * Verify that a social account connection is still valid. * @@ -150,7 +166,7 @@ private function callVerifyEndpoint(SocialAccount $account): bool */ public function refreshToken(SocialAccount $account): bool { - $lock = Cache::lock("token_refresh:{$account->id}", 30); + $lock = Cache::lock("token_refresh:{$account->id}", self::REFRESH_LOCK_SECONDS); if (! $lock->get()) { // Another process is already refreshing this token diff --git a/tests/Feature/Commands/RefreshExpiringTokensTest.php b/tests/Feature/Commands/RefreshExpiringTokensTest.php index 957f8f7f6..aed476840 100644 --- a/tests/Feature/Commands/RefreshExpiringTokensTest.php +++ b/tests/Feature/Commands/RefreshExpiringTokensTest.php @@ -127,3 +127,22 @@ Queue::assertNothingPushed(); }); + +test('a backed-up queue cannot stack duplicate refresh jobs for one account', function () { + Queue::fake(); + + SocialAccount::factory()->x()->create([ + 'workspace_id' => Workspace::factory()->create()->id, + 'status' => Status::Connected, + 'token_expires_at' => now()->addMinutes(20), + ]); + + // Two scheduler ticks before the first job got a worker: token_expires_at + // has not moved, so the account is still inside the window. + $this->artisan('social:refresh-expiring-tokens'); + $this->artisan('social:refresh-expiring-tokens'); + + // Each extra job rotates a single-use refresh_token again for nothing, and + // widens the window where a worker death loses the pair. + Queue::assertPushed(RefreshSocialToken::class, 1); +}); diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 65283ad86..6acbbddec 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -443,3 +443,13 @@ expect($this->account->fresh()->status)->toBe(Status::Connected); Queue::assertNotPushed(SendNotification::class); }); + +test('the refresh lock outlives the slowest refresh a provider can make us wait', function () { + // Bluesky refreshes with two sequential calls (refreshSession, then the + // createSession re-auth), each bounded by the framework's connect + read + // timeouts. If the lock expires first, a second process refreshes with the + // same single-use refresh_token and one of the two is rejected. + $worstCaseSeconds = 2 * (30 + 10); + + expect(ConnectionVerifier::REFRESH_LOCK_SECONDS)->toBeGreaterThan($worstCaseSeconds); +}); From ec1ec66862169bd70cabf1a4033f39bdca0efcc9 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 15:59:45 -0300 Subject: [PATCH 07/19] Correct which providers actually single-use their refresh_token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checked each provider's official documentation rather than carrying the assumption forward. LinkedIn does not rotate. Its refresh docs are explicit: "the lifespan or Time To Live (TTL) of the refresh token remains the same as specified in the initial OAuth flow (365 days)" — the same token comes back with a decreasing refresh_token_expires_in, and only the access token is reissued. The claim that it single-uses the token predates this branch, but a docblock added here repeated it. Bluesky does rotate, and belongs in the list instead: com.atproto.server .refreshSession declares refreshJwt as a required output field, so every refresh mints a new one. Verified alongside, all matching what the code already does: X access token 2h, refresh single-use with rotation Bluesky refreshJwt rotates; createSession is rate-limited per handle (30/5min, 300/day), which the fallback re-auth path shares TikTok access 24h, refresh 365d, "may be different — you must use the newly-returned token", which refreshTikTokToken does LinkedIn access 60d, refresh 365d fixed, not rotated Google does not rotate on refresh; 100 refresh tokens per account per client, so the higher refresh rate on YouTube carries no rotation risk --- app/Jobs/RefreshSocialToken.php | 11 +++++++---- app/Services/Social/ConnectionVerifier.php | 9 +++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 348fd8535..83a1d58f4 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -74,10 +74,13 @@ public function handle(ConnectionVerifier $verifier): void /** * A rejected refresh does not on its own mean the connection is dead. - * Providers that single-use their refresh_token (X, LinkedIn) reject one a - * concurrent refresh already consumed while the current access_token keeps - * working, and an account with no refresh_token at all fails here without - * any call being made. PublishToSocialPlatform hard-fails every post for a + * X and Bluesky single-use their refresh_token — X issues a new one and + * invalidates the previous on every refresh, and refreshSession returns a + * mandatory new refreshJwt — so one a concurrent refresh already consumed + * is rejected while the current access_token keeps working. X is also + * documented by its own developer community to invalidate refresh_tokens + * spuriously. An account with no refresh_token at all fails here without + * any call being made at all. PublishToSocialPlatform hard-fails every post for a * TokenExpired account, so disconnecting on a refresh rejection alone kills * posts the access_token would still have published. * diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index a397b3d21..bc877291c 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -52,10 +52,11 @@ public function verify(SocialAccount $account): bool // Hard-expired tokens cannot make API calls — refresh is mandatory. // For tokens that are still valid OR only "expiring soon", try the // verify endpoint FIRST with the current access_token. This avoids - // rotating refresh_tokens unnecessarily — many providers (X v2, - // LinkedIn, etc.) invalidate the previous refresh_token on each - // refresh, so proactive refreshes during races cause false-positive - // disconnects even though the access_token still works fine. + // rotating refresh_tokens unnecessarily — X and Bluesky invalidate the + // previous refresh_token on each refresh, so refreshing during races + // causes false-positive disconnects even though the access_token still + // works fine. (LinkedIn does not: it returns the same refresh_token, + // keeping the TTL from the original authorization.) if ($account->is_token_expired) { return $this->refreshThenVerify($account); } From ec3fe1738080b0a52cc2b6f38d06113eb3e45e7f Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 16:07:35 -0300 Subject: [PATCH 08/19] Read X post metrics from the timeline that already returned them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Analytics fetched the account's timeline for post ids, then turned around and looked the same ids up again through GET /2/tweets purely to read the public_metrics the first request could have returned. The timeline call asked for start_time, end_time and max_results — never tweet.fields. Both endpoints bill per Post returned, so the second pass claimed the same resources a second time, took a second round-trip, and spent a second slice of the same rate limit. For an account with 250 posts in range that is 6 requests where 3 will do. The saving is in round-trips and rate limit rather than dollars: X deduplicates a resource within a 24-hour UTC window, so the second read of an id already read that day is not charged again. But the docs call that a soft guarantee that "may result in resources not being deduplicated" — this stops leaning on it for 250 resources per analytics load. Behaviour is unchanged: same totals, same 5-page ceiling, same empty result when the account posted nothing in range. The page cap is now a named constant, since it bounds what one load can cost as much as how long it takes. Adds the first tests for XAnalytics::getMetrics, covering the totals, the pagination, and that the metrics arrive on the timeline request. --- app/Services/Social/XAnalytics.php | 108 +++++++++++++---------------- tests/Feature/XAnalyticsTest.php | 68 ++++++++++++++++++ 2 files changed, 117 insertions(+), 59 deletions(-) create mode 100644 tests/Feature/XAnalyticsTest.php diff --git a/app/Services/Social/XAnalytics.php b/app/Services/Social/XAnalytics.php index 618a87eff..6833a0882 100644 --- a/app/Services/Social/XAnalytics.php +++ b/app/Services/Social/XAnalytics.php @@ -16,6 +16,12 @@ class XAnalytics { use HasSocialHttpClient; + /** + * Each page is billed per Post returned, so this bounds what one analytics + * load can cost as much as it bounds how long it takes. + */ + private const MAX_TIMELINE_PAGES = 5; + private string $baseUrl; private string $accessToken; @@ -52,27 +58,54 @@ private function fetchMetricsFromApi(SocialAccount $account, CarbonInterface $si $this->accessToken = $account->access_token; - // Fetch recent tweets in the period - $tweetIds = $this->fetchTweetIds($account, $since, $until); + [$totals, $tweetCount] = $this->fetchTimelineMetrics($account, $since, $until); - if (empty($tweetIds)) { + if ($tweetCount === 0) { return []; } - // Fetch public_metrics for those tweets - return $this->fetchTweetMetrics($tweetIds); + return [ + ['label' => __('analytics.metrics.impressions'), 'value' => $totals['impression_count']], + ['label' => __('analytics.metrics.likes'), 'value' => $totals['like_count']], + ['label' => __('analytics.metrics.retweets'), 'value' => $totals['retweet_count']], + ['label' => __('analytics.metrics.replies'), 'value' => $totals['reply_count']], + ['label' => __('analytics.metrics.quotes'), 'value' => $totals['quote_count']], + ['label' => __('analytics.metrics.bookmarks'), 'value' => $totals['bookmark_count']], + ]; } - private function fetchTweetIds(SocialAccount $account, CarbonInterface $since, CarbonInterface $until): array + /** + * Walk the account's timeline, summing each Post's public_metrics as the + * pages come back. + * + * The metrics are requested from the timeline itself rather than looked up + * afterwards from /2/tweets. Both endpoints bill per Post returned, so + * re-reading the same ids only bought a second round-trip and a second + * claim on the same rate limit — the ids were already in hand, and their + * metrics come along for free on the request that fetched them. + * + * @return array{0: array, 1: int} totals, and how many Posts fed them + */ + private function fetchTimelineMetrics(SocialAccount $account, CarbonInterface $since, CarbonInterface $until): array { - $ids = []; + $totals = [ + 'impression_count' => 0, + 'like_count' => 0, + 'retweet_count' => 0, + 'reply_count' => 0, + 'quote_count' => 0, + 'bookmark_count' => 0, + ]; + + $tweetCount = 0; $paginationToken = null; - for ($i = 0; $i < 5; $i++) { + for ($page = 0; $page < self::MAX_TIMELINE_PAGES; $page++) { $params = [ 'start_time' => $since->toIso8601ZuluString(), 'end_time' => $until->toIso8601ZuluString(), 'max_results' => 100, + 'tweet.fields' => 'public_metrics', ]; if ($paginationToken) { @@ -90,10 +123,14 @@ private function fetchTweetIds(SocialAccount $account, CarbonInterface $since, C } $data = $response->json(); - $tweets = data_get($data, 'data', []); - foreach ($tweets as $tweet) { - $ids[] = data_get($tweet, 'id'); + foreach (data_get($data, 'data', []) as $tweet) { + $tweetCount++; + $metrics = data_get($tweet, 'public_metrics', []); + + foreach (array_keys($totals) as $metric) { + $totals[$metric] += (int) data_get($metrics, $metric, 0); + } } $paginationToken = data_get($data, 'meta.next_token'); @@ -103,54 +140,7 @@ private function fetchTweetIds(SocialAccount $account, CarbonInterface $since, C } } - return $ids; - } - - private function fetchTweetMetrics(array $tweetIds): array - { - $totals = [ - 'impression_count' => 0, - 'like_count' => 0, - 'retweet_count' => 0, - 'reply_count' => 0, - 'quote_count' => 0, - 'bookmark_count' => 0, - ]; - - // X API allows max 100 IDs per request - foreach (array_chunk($tweetIds, 100) as $chunk) { - $response = $this->getHttpClient() - ->get("{$this->baseUrl}/tweets", [ - 'ids' => implode(',', $chunk), - 'tweet.fields' => 'public_metrics', - ]); - - if ($response->failed()) { - Log::warning('X tweets metrics fetch failed', [ - 'body' => $this->redactResponseBody($response->body()), - ]); - - continue; - } - - $tweets = data_get($response->json(), 'data', []); - - foreach ($tweets as $tweet) { - $metrics = data_get($tweet, 'public_metrics', []); - foreach ($totals as $key => &$total) { - $total += data_get($metrics, $key, 0); - } - } - } - - return [ - ['label' => __('analytics.metrics.impressions'), 'value' => $totals['impression_count']], - ['label' => __('analytics.metrics.likes'), 'value' => $totals['like_count']], - ['label' => __('analytics.metrics.retweets'), 'value' => $totals['retweet_count']], - ['label' => __('analytics.metrics.replies'), 'value' => $totals['reply_count']], - ['label' => __('analytics.metrics.quotes'), 'value' => $totals['quote_count']], - ['label' => __('analytics.metrics.bookmarks'), 'value' => $totals['bookmark_count']], - ]; + return [$totals, $tweetCount]; } public function fetchPostMetrics(PostPlatform $postPlatform): array diff --git a/tests/Feature/XAnalyticsTest.php b/tests/Feature/XAnalyticsTest.php new file mode 100644 index 000000000..9258ffb57 --- /dev/null +++ b/tests/Feature/XAnalyticsTest.php @@ -0,0 +1,68 @@ +account = SocialAccount::factory()->x()->create([ + 'workspace_id' => Workspace::factory()->create()->id, + 'platform_user_id' => '4242', + 'token_expires_at' => now()->addHours(2), + ]); + $this->api = config('trypost.platforms.x.api'); +}); + +test('metrics come from the timeline itself instead of a second lookup of the same posts', function () { + Http::fake([ + $this->api.'/users/4242/tweets*' => Http::response([ + 'data' => [ + ['id' => '1', 'public_metrics' => ['impression_count' => 100, 'like_count' => 10, 'retweet_count' => 1, 'reply_count' => 2, 'quote_count' => 3, 'bookmark_count' => 4]], + ['id' => '2', 'public_metrics' => ['impression_count' => 200, 'like_count' => 20, 'retweet_count' => 2, 'reply_count' => 4, 'quote_count' => 6, 'bookmark_count' => 8]], + ], + 'meta' => [], + ], 200), + ]); + + $metrics = app(XAnalytics::class)->getMetrics($this->account); + + // Every post returned is a billed Post read. Re-reading the same ids from + // /2/tweets buys nothing the timeline could not have returned. + Http::assertNotSent(fn ($request) => str_contains($request->url(), '/2/tweets?') + || preg_match('#/tweets\?ids=#', $request->url()) === 1); + + expect(collect($metrics)->firstWhere('label', __('analytics.metrics.impressions'))['value'])->toBe(300); + expect(collect($metrics)->firstWhere('label', __('analytics.metrics.likes'))['value'])->toBe(30); +}); + +test('the timeline request asks for public_metrics', function () { + Http::fake([ + $this->api.'/users/4242/tweets*' => Http::response(['data' => [], 'meta' => []], 200), + ]); + + app(XAnalytics::class)->getMetrics($this->account); + + Http::assertSent(fn ($request) => str_contains($request->url(), 'tweet.fields=public_metrics')); +}); + +test('metrics accumulate across paginated timeline pages', function () { + Http::fake([ + $this->api.'/users/4242/tweets*' => Http::sequence() + ->push([ + 'data' => [['id' => '1', 'public_metrics' => ['impression_count' => 100, 'like_count' => 1, 'retweet_count' => 0, 'reply_count' => 0, 'quote_count' => 0, 'bookmark_count' => 0]]], + 'meta' => ['next_token' => 'page2'], + ], 200) + ->push([ + 'data' => [['id' => '2', 'public_metrics' => ['impression_count' => 50, 'like_count' => 2, 'retweet_count' => 0, 'reply_count' => 0, 'quote_count' => 0, 'bookmark_count' => 0]]], + 'meta' => [], + ], 200), + ]); + + $metrics = app(XAnalytics::class)->getMetrics($this->account); + + expect(collect($metrics)->firstWhere('label', __('analytics.metrics.impressions'))['value'])->toBe(150); + expect(collect($metrics)->firstWhere('label', __('analytics.metrics.likes'))['value'])->toBe(3); +}); From 280407da20852854ebd22a5f5246abb8047dc317 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 16:16:46 -0300 Subject: [PATCH 09/19] Cover the analytics paths a happy-path test walks straight past MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps in what the previous commit's tests actually assert: A timeline page failing mid-pagination breaks out of the loop and returns whatever was collected. Nothing pinned that — partial data beats an exception on a dashboard someone is looking at, and a future refactor could quietly turn it into one. A post can come back without public_metrics. The accumulator defaults each metric to 0, so it contributes nothing instead of erroring, which also wasn't covered. An account with no posts in range returns [] rather than a list of zeros, so the UI can tell "nothing posted" from "posted, no engagement". Also makes the routing test's mock return explicit. Without andReturn, Mockery hands back a falsy default for the new bool return type, so the assertion about routing was passing while silently exercising the lock-skipped branch. The test still asserts only what its name claims, but no longer depends on a mock default to get there. --- tests/Feature/Jobs/RefreshSocialTokenTest.php | 2 +- tests/Feature/XAnalyticsTest.php | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 6acbbddec..5b7a422ee 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -32,7 +32,7 @@ $verifier = mock(ConnectionVerifier::class); $verifier->shouldReceive('refreshToken')->once()->with( Mockery::on(fn ($account) => $account->id === $this->account->id) - ); + )->andReturnTrue(); $verifier->shouldNotReceive('verify'); app()->instance(ConnectionVerifier::class, $verifier); diff --git a/tests/Feature/XAnalyticsTest.php b/tests/Feature/XAnalyticsTest.php index 9258ffb57..5c6e31511 100644 --- a/tests/Feature/XAnalyticsTest.php +++ b/tests/Feature/XAnalyticsTest.php @@ -66,3 +66,44 @@ expect(collect($metrics)->firstWhere('label', __('analytics.metrics.impressions'))['value'])->toBe(150); expect(collect($metrics)->firstWhere('label', __('analytics.metrics.likes'))['value'])->toBe(3); }); + +test('a page that fails mid-pagination keeps the totals collected so far', function () { + Http::fake([ + $this->api.'/users/4242/tweets*' => Http::sequence() + ->push([ + 'data' => [['id' => '1', 'public_metrics' => ['impression_count' => 100, 'like_count' => 5, 'retweet_count' => 0, 'reply_count' => 0, 'quote_count' => 0, 'bookmark_count' => 0]]], + 'meta' => ['next_token' => 'page2'], + ], 200) + ->push(['title' => 'Internal Error'], 500), + ]); + + $metrics = app(XAnalytics::class)->getMetrics($this->account); + + // Partial data beats an exception on a dashboard the user is looking at. + expect(collect($metrics)->firstWhere('label', __('analytics.metrics.impressions'))['value'])->toBe(100); + expect(collect($metrics)->firstWhere('label', __('analytics.metrics.likes'))['value'])->toBe(5); +}); + +test('a post returned without public_metrics counts as zero rather than erroring', function () { + Http::fake([ + $this->api.'/users/4242/tweets*' => Http::response([ + 'data' => [ + ['id' => '1'], + ['id' => '2', 'public_metrics' => ['impression_count' => 7, 'like_count' => 1, 'retweet_count' => 0, 'reply_count' => 0, 'quote_count' => 0, 'bookmark_count' => 0]], + ], + 'meta' => [], + ], 200), + ]); + + $metrics = app(XAnalytics::class)->getMetrics($this->account); + + expect(collect($metrics)->firstWhere('label', __('analytics.metrics.impressions'))['value'])->toBe(7); +}); + +test('an account that posted nothing in the range returns no metrics at all', function () { + Http::fake([ + $this->api.'/users/4242/tweets*' => Http::response(['data' => [], 'meta' => []], 200), + ]); + + expect(app(XAnalytics::class)->getMetrics($this->account))->toBe([]); +}); From af379af765fe10e3776f89bb35d4d06e4a9b5e93 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 16:27:52 -0300 Subject: [PATCH 10/19] Close five issues an independent review found in this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All five sit in code these commits introduced. Instagram and Threads must fail loudly. Their long-lived token is extended in place and cannot be renewed once it expires, and RefreshExpiringTokens picks them up a full day ahead precisely so there is time to react. The fallback added in 9775578 applied to them too, so a permanently rejected extension on a token that still reads left the account Connected — the daily sweep passed as well, since verify() succeeds on it — and the owner learned about it only after the token died unrecoverably, while every tick retried the rejection for 24 hours. The fallback now applies only to platforms that rotate a refresh_token, which is what it was written for. The lock went the wrong way. Lengthening it to 120s in 5001c9a treated the scheduler as the only caller, but publishers wait on the same lock: one left behind by a worker that died mid-refresh makes refreshToken() return false, and the publisher falls through and publishes with an expired token. That window was 30s and had become 120s. Bound the calls instead — a token endpoint answers in milliseconds, and 8s read / 4s connect keeps even Bluesky's two sequential calls under a 30-second lock, back to where main had it. Reloading the account can throw. $this->account->refresh() sat outside the try in the fallback, and an exception raised inside a catch block is not caught by a sibling catch. With tries = 1, an account deleted mid-run put the job in failed_jobs. VerifyUpcomingPostConnections guards this same race explicitly. An empty access token was detected but not acted on. recordVerification() declined to stamp it, yet the refresh still counted as a success — and the refresh method had already pushed token_expires_at two hours out, so the account left the window looking healthy while every publish 401d. Mark it expired, which is what verify() used to do on the same input. refreshToken() claimed "whether a refresh actually ran" but returned true for platforms whose match arm does nothing. Only recordVerification() re-checking hasTokenRefreshFlow() kept that from mattering. The guard is now explicit and the contract true at the source. Also settles the tweet.fields question against the live API rather than the docs, which contradict each other: the OpenAPI spec names the parameter post.fields, while the Fields guide and every example use tweet.fields. Both are accepted and both return public_metrics. An unrecognised name returns 200 and silently omits the field — no error — so the name being right is load bearing, and it is. --- app/Jobs/RefreshSocialToken.php | 44 +++++++--- app/Services/Social/ConnectionVerifier.php | 65 +++++++++----- tests/Feature/Jobs/RefreshSocialTokenTest.php | 87 ++++++++++++++++++- 3 files changed, 159 insertions(+), 37 deletions(-) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 83a1d58f4..24af43173 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -49,6 +49,16 @@ public function handle(ConnectionVerifier $verifier): void { try { if ($verifier->refreshToken($this->account)) { + if (blank($this->account->access_token)) { + // The refresh method stored whatever came back and pushed + // token_expires_at forward, so the account would otherwise + // leave the refresh window looking healthy while every + // publish 401s on an empty token. + $this->account->markAsTokenExpired('Token refresh returned an empty access token'); + + return; + } + $this->recordVerification(); } } catch (PlatformUnavailableException $e) { @@ -58,7 +68,7 @@ public function handle(ConnectionVerifier $verifier): void 'error' => $e->getMessage(), ]); } catch (TokenExpiredException $e) { - if ($this->accessTokenStillWorks($verifier)) { + if ($this->shouldTrustAWorkingAccessToken() && $this->accessTokenStillWorks($verifier)) { return; } @@ -95,9 +105,11 @@ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool // flight, which is why ours was rejected. This instance still holds the // token that was rotated away, so reload before judging it — otherwise // the winner's healthy account gets disconnected. - $this->account->refresh(); - try { + // Inside the try: the account can be deleted mid-run, and this job + // has tries = 1, so an escaping ModelNotFoundException fails it. + $this->account->refresh(); + return $verifier->verifyAccessToken($this->account); } catch (TokenExpiredException) { return false; @@ -112,13 +124,29 @@ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool } } + /** + * Whether a working access token is reason enough to stay connected after a + * refresh was rejected. + * + * It is for platforms that rotate a refresh_token, where a rejection often + * just means we lost a race and the current token is fine. It is not for + * Instagram and Threads: their long-lived token is extended in place and + * cannot be renewed once it expires, so a permanently rejected extension + * means the connection is already doomed. Staying connected because the + * token still reads would tell the owner only after it dies, when + * reconnecting is the only option left, and would keep retrying the + * rejected extension every 15 minutes across the whole 24-hour lead. + */ + private function shouldTrustAWorkingAccessToken(): bool + { + return ! $this->account->platform->extendsAccessTokenOnRefresh(); + } + /** * Record the refresh as a verification, so the daily sweep and the * pre-publish check can skip their own (often billed) verify call. * - * Only a refresh that came back with a usable token proves anything: TokenRefreshClient classifies on HTTP - * status alone and never inspects the body, so a 200 carrying an empty - * token would otherwise be recorded as healthy. This is deliberately not + * Deliberately not * done inside ConnectionVerifier::refreshToken() — refreshThenVerify() * calls it and can still fail on the verify that follows, and a stamp * written there would vouch for a credential nothing ever confirmed. @@ -129,10 +157,6 @@ private function recordVerification(): void return; } - if (blank($this->account->access_token)) { - return; - } - $this->account->update(['last_verified_at' => now()]); } } diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index bc877291c..c437a840a 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -20,26 +20,35 @@ use App\Services\Social\Discord\DiscordClient; use App\Services\Social\Meta\GraphError; use App\Services\Social\Telegram\TelegramApi; +use Illuminate\Http\Client\PendingRequest; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; class ConnectionVerifier { /** - * How long the per-account refresh lock survives without being released. + * Read and connect timeouts for a token refresh. * - * It has to outlast the slowest refresh a provider can make us wait for, - * or the lock lapses mid-flight and a second process refreshes with the - * same single-use refresh_token — leaving one of the two rejected. The - * ceiling is Bluesky, which refreshes with two sequential calls - * (refreshSession, then the createSession re-auth), each bounded by the - * HTTP client's connect and read timeouts. + * Bounding the call ourselves is what keeps a refresh under the lock that + * protects it. The HTTP client's defaults (30s read, 10s connect) let a + * single Bluesky refresh — two sequential calls — run for ~80 seconds under + * a 30-second lock, so the lock could lapse mid-flight and let a second + * process refresh with the same single-use refresh_token. * - * The lock is released in a finally block, so this only governs how long a - * worker that died mid-refresh blocks the next attempt — and the next - * scheduler tick is 15 minutes out either way. + * Fixing that by holding the lock longer would have been worse: publishers + * wait on the same lock, and a lock left behind by a worker that died + * mid-refresh makes them fall through and publish with an expired token. + * A token endpoint answers in milliseconds, so bounding the call is free. */ - public const REFRESH_LOCK_SECONDS = 120; + public const REFRESH_TIMEOUT_SECONDS = 8; + + public const REFRESH_CONNECT_TIMEOUT_SECONDS = 4; + + /** + * Must exceed the slowest refresh the timeouts above allow — Bluesky's two + * sequential calls. Pinned by a test so the two can't drift apart. + */ + public const REFRESH_LOCK_SECONDS = 30; /** * Verify that a social account connection is still valid. @@ -113,6 +122,12 @@ private function refreshThenVerify(SocialAccount $account, ?TokenExpiredExceptio } } + private function refreshHttp(): PendingRequest + { + return Http::timeout(self::REFRESH_TIMEOUT_SECONDS) + ->connectTimeout(self::REFRESH_CONNECT_TIMEOUT_SECONDS); + } + /** * Check the stored access token exactly as it is, skipping the * refresh-and-retry ladder verify() runs. @@ -177,6 +192,13 @@ public function refreshToken(SocialAccount $account): bool } try { + if (! $account->platform->hasTokenRefreshFlow()) { + // Facebook / InstagramFacebook use Page tokens that don't + // expire, Mastodon's don't either, and Telegram and Discord + // share one bot token with nothing per-account to refresh. + return false; + } + match ($account->platform) { Platform::LinkedIn, Platform::LinkedInPage => $this->refreshLinkedInToken($account), Platform::X => $this->refreshXToken($account), @@ -186,9 +208,6 @@ public function refreshToken(SocialAccount $account): bool Platform::Pinterest => $this->refreshPinterestToken($account), Platform::Threads => $this->refreshThreadsToken($account), Platform::Instagram => $this->refreshInstagramToken($account), - // Facebook / InstagramFacebook use Page tokens that don't expire. - // Mastodon tokens don't expire either. - default => null, }; return true; @@ -203,7 +222,7 @@ private function refreshLinkedInToken(SocialAccount $account): void throw new TokenExpiredException("No refresh token available for {$account->platform->label()} account"); } - $response = TokenRefreshClient::for($account->platform)->send(fn () => Http::asForm() + $response = TokenRefreshClient::for($account->platform)->send(fn () => $this->refreshHttp()->asForm() ->post(config('trypost.platforms.linkedin.oauth_api').'/oauth/v2/accessToken', [ 'grant_type' => 'refresh_token', 'refresh_token' => $account->refresh_token, @@ -228,7 +247,7 @@ private function refreshXToken(SocialAccount $account): void throw new TokenExpiredException('No refresh token available for X account'); } - $response = TokenRefreshClient::for(Platform::X)->send(fn () => Http::asForm() + $response = TokenRefreshClient::for(Platform::X)->send(fn () => $this->refreshHttp()->asForm() ->withBasicAuth(config('services.x.client_id'), config('services.x.client_secret')) ->post(config('trypost.platforms.x.api').'/oauth2/token', [ 'grant_type' => 'refresh_token', @@ -252,7 +271,7 @@ private function refreshBlueskyToken(SocialAccount $account): void $client = TokenRefreshClient::for(Platform::Bluesky); try { - $response = $client->send(fn () => Http::withToken($account->refresh_token) + $response = $client->send(fn () => $this->refreshHttp()->withToken($account->refresh_token) ->post("{$service}/xrpc/".BlueskyLexicon::REFRESH_SESSION)); $data = $response->json(); @@ -271,7 +290,7 @@ private function refreshBlueskyToken(SocialAccount $account): void if (isset($account->meta['password'])) { try { - $reauth = $client->send(fn () => Http::post("{$service}/xrpc/".BlueskyLexicon::CREATE_SESSION, [ + $reauth = $client->send(fn () => $this->refreshHttp()->post("{$service}/xrpc/".BlueskyLexicon::CREATE_SESSION, [ 'identifier' => $account->meta['identifier'], 'password' => decrypt($account->meta['password']), ])); @@ -300,7 +319,7 @@ private function refreshYouTubeToken(SocialAccount $account): void throw new TokenExpiredException('No refresh token available for YouTube account'); } - $response = TokenRefreshClient::for(Platform::YouTube)->send(fn () => Http::asForm() + $response = TokenRefreshClient::for(Platform::YouTube)->send(fn () => $this->refreshHttp()->asForm() ->post(config('trypost.platforms.youtube.oauth_api').'/token', [ 'grant_type' => 'refresh_token', 'refresh_token' => $account->refresh_token, @@ -324,7 +343,7 @@ private function refreshTikTokToken(SocialAccount $account): void throw new TokenExpiredException('No refresh token available for TikTok account'); } - $response = TokenRefreshClient::for(Platform::TikTok)->send(fn () => Http::asForm() + $response = TokenRefreshClient::for(Platform::TikTok)->send(fn () => $this->refreshHttp()->asForm() ->post(config('trypost.platforms.tiktok.api').'/oauth/token/', [ 'grant_type' => 'refresh_token', 'refresh_token' => $account->refresh_token, @@ -351,7 +370,7 @@ private function refreshPinterestToken(SocialAccount $account): void $credentials = base64_encode(config('services.pinterest.client_id').':'.config('services.pinterest.client_secret')); - $response = TokenRefreshClient::for(Platform::Pinterest)->send(fn () => Http::withHeaders([ + $response = TokenRefreshClient::for(Platform::Pinterest)->send(fn () => $this->refreshHttp()->withHeaders([ 'Authorization' => "Basic {$credentials}", 'Content-Type' => 'application/x-www-form-urlencoded', ])->asForm()->post(config('trypost.platforms.pinterest.api').'/oauth/token', [ @@ -374,7 +393,7 @@ private function refreshThreadsToken(SocialAccount $account): void { // Threads uses long-lived tokens that can be refreshed $response = TokenRefreshClient::for(Platform::Threads)->send( - fn () => Http::get(config('trypost.platforms.threads.auth_api').'/refresh_access_token', [ + fn () => $this->refreshHttp()->get(config('trypost.platforms.threads.auth_api').'/refresh_access_token', [ 'grant_type' => 'th_refresh_token', 'access_token' => $account->access_token, ]), @@ -396,7 +415,7 @@ private function refreshThreadsToken(SocialAccount $account): void private function refreshInstagramToken(SocialAccount $account): void { $response = TokenRefreshClient::for(Platform::Instagram)->send( - fn () => Http::get(config('trypost.platforms.instagram.auth_api').'/refresh_access_token', [ + fn () => $this->refreshHttp()->get(config('trypost.platforms.instagram.auth_api').'/refresh_access_token', [ 'grant_type' => 'ig_refresh_token', 'access_token' => $account->access_token, ]), diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 5b7a422ee..f99660fcb 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -446,10 +446,89 @@ test('the refresh lock outlives the slowest refresh a provider can make us wait', function () { // Bluesky refreshes with two sequential calls (refreshSession, then the - // createSession re-auth), each bounded by the framework's connect + read - // timeouts. If the lock expires first, a second process refreshes with the - // same single-use refresh_token and one of the two is rejected. - $worstCaseSeconds = 2 * (30 + 10); + // createSession re-auth). If the lock expires first, a second process + // refreshes with the same single-use refresh_token and one of the two is + // rejected. Bounding the calls ourselves keeps that under the lock without + // holding the lock longer, which the publish path also waits on. + $worstCaseSeconds = 2 * (ConnectionVerifier::REFRESH_TIMEOUT_SECONDS + ConnectionVerifier::REFRESH_CONNECT_TIMEOUT_SECONDS); expect(ConnectionVerifier::REFRESH_LOCK_SECONDS)->toBeGreaterThan($worstCaseSeconds); }); + +test('a rejected Instagram extension disconnects loudly instead of waiting for the token to die', function () { + Queue::fake(); + + $account = SocialAccount::factory()->create([ + 'workspace_id' => $this->workspace->id, + 'platform' => Platform::Instagram, + 'status' => Status::Connected, + 'access_token' => 'still-valid-but-unextendable', + 'token_expires_at' => now()->addHours(20), + ]); + + Http::fake([ + config('trypost.platforms.instagram.auth_api').'/refresh_access_token*' => Http::response([ + 'error' => ['message' => 'Invalid OAuth access token', 'type' => 'OAuthException', 'code' => 190], + ], 400), + config('trypost.platforms.instagram.graph_api').'/me*' => Http::response(['id' => '1', 'username' => 'u'], 200), + ]); + + (new RefreshSocialToken($account))->handle(app(ConnectionVerifier::class)); + + // Instagram/Threads tokens cannot be refreshed once expired. Staying + // Connected because the token still reads means the owner is told only + // after it dies — by which point reconnecting is the only option left. + expect($account->fresh()->status)->toBe(Status::TokenExpired); + Queue::assertPushed(SendNotification::class); +}); + +test('the job survives the account being deleted while it is in flight', function () { + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response(['error' => 'invalid_grant'], 400), + ]); + + $account = $this->account; + $account->update(['token_expires_at' => now()->addMinutes(20)]); + + SocialAccount::whereKey($account->id)->delete(); + + // Guard the repro itself: a delete that silently did nothing would make + // this test pass without ever exercising the path it claims to cover. + expect(SocialAccount::find($account->id))->toBeNull(); + + // tries = 1, so an escaping exception lands the job straight in failed_jobs. + (new RefreshSocialToken($account))->handle(app(ConnectionVerifier::class)); + + // Reaching this line is the point: the refresh ran and the vanished row + // did not escape as a ModelNotFoundException. + Http::assertSent(fn ($request) => str_contains($request->url(), '/oauth2/token')); +}); + +test('a refresh that returns an empty access token disconnects instead of looking healthy', function () { + Queue::fake(); + + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'access_token' => '', + 'refresh_token' => 'rt-new', + 'expires_in' => 7200, + ], 200), + ]); + + $this->account->update(['token_expires_at' => now()->addMinutes(20)]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + // token_expires_at was just pushed 2h out, so the account would otherwise + // leave the refresh window looking healthy while every publish 401s. + expect($this->account->fresh()->status)->toBe(Status::TokenExpired); +}); + +test('refreshToken reports false for a platform with nothing to refresh', function () { + $account = SocialAccount::factory()->mastodon()->create([ + 'workspace_id' => $this->workspace->id, + 'status' => Status::Connected, + ]); + + expect(app(ConnectionVerifier::class)->refreshToken($account))->toBeFalse(); +}); From fb22c41f01a3cb54ce100cf98ad4eaaa21718eaf Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 16:32:27 -0300 Subject: [PATCH 11/19] Guard the match that no longer has a default arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dropping `default => null` from refreshToken() in af379af made the return value honest, but it also turned a missing case into an UnhandledMatchError at runtime. The arms and hasTokenRefreshFlow() currently agree on the same nine platforms, and nothing enforces that: adding a platform to the predicate without an arm would fail in production, on a queue worker, for one platform's accounts only. The test walks every platform claiming a refresh flow and fails by name if the match has no arm for it. Verified it catches the real thing by temporarily adding Facebook to the predicate — it failed with "facebook claims a refresh flow but refreshToken() has no arm for it" — rather than trusting a green run on code that already agrees with itself. --- tests/Feature/Jobs/RefreshSocialTokenTest.php | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index f99660fcb..1a10c7653 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -532,3 +532,43 @@ expect(app(ConnectionVerifier::class)->refreshToken($account))->toBeFalse(); }); + +test('every platform that claims a refresh flow has one implemented', function () { + Http::fake(['*' => Http::response([ + 'access_token' => 'at', 'refresh_token' => 'rt', 'expires_in' => 3600, + 'accessJwt' => 'j', 'refreshJwt' => 'r', 'id' => '1', 'data' => ['id' => '1'], + ], 200)]); + + $verifier = app(ConnectionVerifier::class); + $checked = 0; + + foreach (Platform::cases() as $platform) { + if (! $platform->hasTokenRefreshFlow()) { + continue; + } + + $checked++; + + $account = SocialAccount::factory()->create([ + 'workspace_id' => Workspace::factory()->create()->id, + 'platform' => $platform, + 'status' => Status::Connected, + 'refresh_token' => 'rt-seed', + 'meta' => ['service' => 'https://bsky.social', 'identifier' => 'a.bsky.social'], + ]); + + try { + $verifier->refreshToken($account); + } catch (UnhandledMatchError $e) { + // refreshToken()'s match has no default arm, so a platform added to + // hasTokenRefreshFlow() without one blows up in production instead + // of falling through quietly. + $this->fail("{$platform->value} claims a refresh flow but refreshToken() has no arm for it"); + } catch (Throwable) { + // Provider-specific failures are irrelevant here; only the missing + // arm is what this guards. + } + } + + expect($checked)->toBeGreaterThan(0); +}); From 609a56e2d410c822c09f2538c2af3201609bd195 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 16:37:49 -0300 Subject: [PATCH 12/19] Make the per-platform guard fail on a broken client chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test added in the previous commit swallowed every Throwable except UnhandledMatchError, so it proved a match arm existed and nothing more. Routing all nine refresh methods through refreshHttp() in af379af rewrote how each one builds its request, and this test would have passed just the same if one of those chains no longer worked. It now fails on any exception, naming the platform, and asserts each refresh actually put a request on the wire — a chain that breaks during construction raises before anything is sent, so an empty recording is the signal. Verified by breaking Pinterest's chain on purpose: "pinterest refresh threw BadMethodCallException: Method PendingRequest::withHeadersTypo does not exist." All nine send their request with the chains as they stand. --- tests/Feature/Jobs/RefreshSocialTokenTest.php | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 1a10c7653..059723e5d 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -533,12 +533,7 @@ expect(app(ConnectionVerifier::class)->refreshToken($account))->toBeFalse(); }); -test('every platform that claims a refresh flow has one implemented', function () { - Http::fake(['*' => Http::response([ - 'access_token' => 'at', 'refresh_token' => 'rt', 'expires_in' => 3600, - 'accessJwt' => 'j', 'refreshJwt' => 'r', 'id' => '1', 'data' => ['id' => '1'], - ], 200)]); - +test('every platform that claims a refresh flow actually performs one', function () { $verifier = app(ConnectionVerifier::class); $checked = 0; @@ -557,6 +552,11 @@ 'meta' => ['service' => 'https://bsky.social', 'identifier' => 'a.bsky.social'], ]); + Http::fake(['*' => Http::response([ + 'access_token' => 'at', 'refresh_token' => 'rt', 'expires_in' => 3600, + 'accessJwt' => 'j', 'refreshJwt' => 'r', 'id' => '1', 'data' => ['id' => '1'], + ], 200)]); + try { $verifier->refreshToken($account); } catch (UnhandledMatchError $e) { @@ -564,10 +564,15 @@ // hasTokenRefreshFlow() without one blows up in production instead // of falling through quietly. $this->fail("{$platform->value} claims a refresh flow but refreshToken() has no arm for it"); - } catch (Throwable) { - // Provider-specific failures are irrelevant here; only the missing - // arm is what this guards. + } catch (Throwable $e) { + $this->fail("{$platform->value} refresh threw ".$e::class.': '.$e->getMessage()); } + + // A broken client chain (a renamed helper, a method that no longer + // exists on PendingRequest) raises before anything leaves the process, + // so "no request sent" is the signal that catches it. + expect(Http::recorded()) + ->not->toBeEmpty("{$platform->value} refresh sent no HTTP request at all"); } expect($checked)->toBeGreaterThan(0); From 7a7d964d3cf461cbc48d1f99498db4179a6dac63 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 16:43:04 -0300 Subject: [PATCH 13/19] Stop trading a recoverable failure for an unrecoverable one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A max-effort review found six issues, three of which undo a trade the previous round got backwards. Every refresh method wrote the response straight over the stored credential: `'access_token' => data_get($data, 'access_token')`. A 200 carrying no token therefore destroyed a working one — and on Instagram and Threads, where refresh_token is set to the same value, both halves at once. The blank() check added in af379af only noticed after the damage was persisted, then marked the account TokenExpired, which RefreshExpiringTokens no longer selects — so one glitchy-but-successful response emailed the owner and forced a manual reconnect. Guard before the write instead, treat it as the platform misbehaving, and the stored pair survives for the next tick to retry. The detection branch downstream is now unreachable and gone. Tightening the refresh timeout to 8s was the wrong fix for the lock problem. refreshHttp() is shared with 24 publish and analytics call sites, and for X, Bluesky and TikTok the refresh_token is single-use: abandoning a request the provider has already processed loses the rotated pair permanently and costs the user a reconnect. Giving up sooner makes that more likely, not less. Restore generous timeouts and put the lock back above them. The cost of erring long is that a worker dying mid-refresh holds the lock while a publish falls through and retries — recoverable, unlike a lost rotation. Both constants now say which way they are wrong on purpose. The hasTokenRefreshFlow() guard in recordVerification() was dead: refreshToken() already returns false for those platforms, so the branch was never entered. The test claiming to cover it calls refreshToken() directly and never reaches it. The command reported "Dispatched N" for a number it cannot know. dispatch() returns a PendingDispatch whether or not ShouldBeUnique discarded it, so the count overstated itself during exactly the backlog someone reads that line to diagnose. It now reports accounts in the window, which is what it actually measured. Not changed: the daily sweep still skips accounts a refresh keeps fresh. That is the deliberate decision this PR is built on — a refresh replaces the access token rather than inspecting it, so there is nothing left for a billed read to confirm. Re-verified live after the changes: the real job still rotates the token against api.x.com and leaves the account connected. --- .../Commands/RefreshExpiringTokens.php | 6 +- app/Jobs/RefreshSocialToken.php | 17 +--- app/Services/Social/ConnectionVerifier.php | 79 +++++++++++++------ .../Commands/RefreshExpiringTokensTest.php | 16 ++++ tests/Feature/Jobs/RefreshSocialTokenTest.php | 18 +++-- 5 files changed, 90 insertions(+), 46 deletions(-) diff --git a/app/Console/Commands/RefreshExpiringTokens.php b/app/Console/Commands/RefreshExpiringTokens.php index b6906ac07..0901c6578 100644 --- a/app/Console/Commands/RefreshExpiringTokens.php +++ b/app/Console/Commands/RefreshExpiringTokens.php @@ -46,6 +46,10 @@ public function handle(): void } }); - $this->info("Dispatched {$count} token refresh jobs."); + // Accounts in the window, not jobs queued: RefreshSocialToken is unique + // per account, so a dispatch is discarded while another is still in + // flight — and a dispatched count would overstate itself during exactly + // the backlog an operator reads this line to diagnose. + $this->info("{$count} accounts due for a token refresh."); } } diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 24af43173..182f6c02d 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -49,16 +49,6 @@ public function handle(ConnectionVerifier $verifier): void { try { if ($verifier->refreshToken($this->account)) { - if (blank($this->account->access_token)) { - // The refresh method stored whatever came back and pushed - // token_expires_at forward, so the account would otherwise - // leave the refresh window looking healthy while every - // publish 401s on an empty token. - $this->account->markAsTokenExpired('Token refresh returned an empty access token'); - - return; - } - $this->recordVerification(); } } catch (PlatformUnavailableException $e) { @@ -146,17 +136,14 @@ private function shouldTrustAWorkingAccessToken(): bool * Record the refresh as a verification, so the daily sweep and the * pre-publish check can skip their own (often billed) verify call. * - * Deliberately not + * refreshToken() reports whether one actually ran, so a lock skipped by a + * concurrent refresh never reaches here. Deliberately not * done inside ConnectionVerifier::refreshToken() — refreshThenVerify() * calls it and can still fail on the verify that follows, and a stamp * written there would vouch for a credential nothing ever confirmed. */ private function recordVerification(): void { - if (! $this->account->platform->hasTokenRefreshFlow()) { - return; - } - $this->account->update(['last_verified_at' => now()]); } } diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index c437a840a..79663b635 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -29,26 +29,28 @@ class ConnectionVerifier /** * Read and connect timeouts for a token refresh. * - * Bounding the call ourselves is what keeps a refresh under the lock that - * protects it. The HTTP client's defaults (30s read, 10s connect) let a - * single Bluesky refresh — two sequential calls — run for ~80 seconds under - * a 30-second lock, so the lock could lapse mid-flight and let a second - * process refresh with the same single-use refresh_token. - * - * Fixing that by holding the lock longer would have been worse: publishers - * wait on the same lock, and a lock left behind by a worker that died - * mid-refresh makes them fall through and publish with an expired token. - * A token endpoint answers in milliseconds, so bounding the call is free. + * Deliberately generous. refreshHttp() is shared with the ~24 publish and + * analytics call sites, and for X, Bluesky and TikTok the refresh_token is + * single-use: giving up on a request the provider has already processed + * loses the rotated pair for good and costs the user a manual reconnect. + * A token endpoint answers in milliseconds, so a long ceiling is nearly + * free, while a tight one turns provider slowness into dead accounts. */ - public const REFRESH_TIMEOUT_SECONDS = 8; + public const REFRESH_TIMEOUT_SECONDS = 30; - public const REFRESH_CONNECT_TIMEOUT_SECONDS = 4; + public const REFRESH_CONNECT_TIMEOUT_SECONDS = 10; /** * Must exceed the slowest refresh the timeouts above allow — Bluesky's two - * sequential calls. Pinned by a test so the two can't drift apart. + * sequential calls — or the lock lapses mid-flight and a second process + * refreshes with the same single-use refresh_token. Pinned by a test. + * + * The cost of erring long is that a worker dying mid-refresh leaves the + * lock held for this many seconds, during which a publish falls through to + * an expired token and retries. That is recoverable; an abandoned rotation + * is not. */ - public const REFRESH_LOCK_SECONDS = 30; + public const REFRESH_LOCK_SECONDS = 120; /** * Verify that a social account connection is still valid. @@ -122,6 +124,33 @@ private function refreshThenVerify(SocialAccount $account, ?TokenExpiredExceptio } } + /** + * Pull a token out of a refresh response, refusing to persist a blank one. + * + * TokenRefreshClient classifies on HTTP status alone and never inspects the + * body, so a 200 carrying no token would otherwise be written straight over + * a credential that still works — and on Instagram and Threads, where the + * refresh_token is set to the same value, both halves go at once. Treat it + * as the platform misbehaving: the stored pair stays put and the next tick + * retries, instead of the account needing a manual reconnect. + * + * @param array|null $data + * + * @throws PlatformUnavailableException + */ + private function tokenFrom(?array $data, Platform $platform, string $key = 'access_token'): string + { + $token = data_get($data, $key); + + if (blank($token)) { + throw new PlatformUnavailableException( + "{$platform->label()} returned a successful refresh with no {$key}." + ); + } + + return (string) $token; + } + private function refreshHttp(): PendingRequest { return Http::timeout(self::REFRESH_TIMEOUT_SECONDS) @@ -233,7 +262,7 @@ private function refreshLinkedInToken(SocialAccount $account): void $data = $response->json(); $account->update([ - 'access_token' => data_get($data, 'access_token'), + 'access_token' => $this->tokenFrom($data, $account->platform), 'refresh_token' => data_get($data, 'refresh_token', $account->refresh_token), 'token_expires_at' => data_get($data, 'expires_in') ? now()->addSeconds(data_get($data, 'expires_in')) : null, ]); @@ -257,7 +286,7 @@ private function refreshXToken(SocialAccount $account): void $data = $response->json(); $account->update([ - 'access_token' => data_get($data, 'access_token'), + 'access_token' => $this->tokenFrom($data, $account->platform), 'refresh_token' => data_get($data, 'refresh_token', $account->refresh_token), 'token_expires_at' => now()->addSeconds(data_get($data, 'expires_in', $account->platform->defaultTokenTtlSeconds())), ]); @@ -276,8 +305,8 @@ private function refreshBlueskyToken(SocialAccount $account): void $data = $response->json(); $account->update([ - 'access_token' => data_get($data, 'accessJwt'), - 'refresh_token' => data_get($data, 'refreshJwt'), + 'access_token' => $this->tokenFrom($data, $account->platform, 'accessJwt'), + 'refresh_token' => $this->tokenFrom($data, $account->platform, 'refreshJwt'), 'token_expires_at' => now()->addHours(2), ]); @@ -297,8 +326,8 @@ private function refreshBlueskyToken(SocialAccount $account): void $data = $reauth->json(); $account->update([ - 'access_token' => data_get($data, 'accessJwt'), - 'refresh_token' => data_get($data, 'refreshJwt'), + 'access_token' => $this->tokenFrom($data, $account->platform, 'accessJwt'), + 'refresh_token' => $this->tokenFrom($data, $account->platform, 'refreshJwt'), 'token_expires_at' => now()->addHours(2), ]); @@ -330,7 +359,7 @@ private function refreshYouTubeToken(SocialAccount $account): void $data = $response->json(); $account->update([ - 'access_token' => data_get($data, 'access_token'), + 'access_token' => $this->tokenFrom($data, $account->platform), 'token_expires_at' => data_get($data, 'expires_in') ? now()->addSeconds(data_get($data, 'expires_in')) : null, ]); @@ -354,7 +383,7 @@ private function refreshTikTokToken(SocialAccount $account): void $data = $response->json(); $account->update([ - 'access_token' => data_get($data, 'access_token'), + 'access_token' => $this->tokenFrom($data, $account->platform), 'refresh_token' => data_get($data, 'refresh_token', $account->refresh_token), 'token_expires_at' => data_get($data, 'expires_in') ? now()->addSeconds(data_get($data, 'expires_in')) : null, ]); @@ -381,7 +410,7 @@ private function refreshPinterestToken(SocialAccount $account): void $data = $response->json(); $account->update([ - 'access_token' => data_get($data, 'access_token'), + 'access_token' => $this->tokenFrom($data, $account->platform), 'refresh_token' => data_get($data, 'refresh_token', $account->refresh_token), 'token_expires_at' => data_get($data, 'expires_in') ? now()->addSeconds(data_get($data, 'expires_in')) : null, ]); @@ -401,7 +430,7 @@ private function refreshThreadsToken(SocialAccount $account): void ); $data = $response->json(); - $newToken = data_get($data, 'access_token'); + $newToken = $this->tokenFrom($data, $account->platform); $account->update([ 'access_token' => $newToken, @@ -423,7 +452,7 @@ private function refreshInstagramToken(SocialAccount $account): void ); $data = $response->json(); - $newToken = data_get($data, 'access_token'); + $newToken = $this->tokenFrom($data, $account->platform); $account->update([ 'access_token' => $newToken, diff --git a/tests/Feature/Commands/RefreshExpiringTokensTest.php b/tests/Feature/Commands/RefreshExpiringTokensTest.php index aed476840..0eb389bfe 100644 --- a/tests/Feature/Commands/RefreshExpiringTokensTest.php +++ b/tests/Feature/Commands/RefreshExpiringTokensTest.php @@ -146,3 +146,19 @@ // widens the window where a worker death loses the pair. Queue::assertPushed(RefreshSocialToken::class, 1); }); + +test('the command reports accounts in the window, not jobs it cannot know landed', function () { + Queue::fake(); + + SocialAccount::factory()->x()->create([ + 'workspace_id' => Workspace::factory()->create()->id, + 'status' => Status::Connected, + 'token_expires_at' => now()->addMinutes(20), + ]); + + // RefreshSocialToken is unique per account, so a second dispatch while the + // first is in flight is silently discarded. dispatch() still returns a + // PendingDispatch either way, so a "dispatched" count would be a guess. + $this->artisan('social:refresh-expiring-tokens') + ->expectsOutput('1 accounts due for a token refresh.'); +}); diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 059723e5d..b1015fa84 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -504,7 +504,7 @@ Http::assertSent(fn ($request) => str_contains($request->url(), '/oauth2/token')); }); -test('a refresh that returns an empty access token disconnects instead of looking healthy', function () { +test('a 200 without a token leaves the working credential intact', function () { Queue::fake(); Http::fake([ @@ -515,13 +515,21 @@ ], 200), ]); - $this->account->update(['token_expires_at' => now()->addMinutes(20)]); + $this->account->update([ + 'access_token' => 'the-token-that-still-works', + 'token_expires_at' => now()->addMinutes(20), + ]); (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); - // token_expires_at was just pushed 2h out, so the account would otherwise - // leave the refresh window looking healthy while every publish 401s. - expect($this->account->fresh()->status)->toBe(Status::TokenExpired); + // Persisting the empty token would destroy a credential that still works, + // and no amount of after-the-fact detection gets it back. + expect($this->account->fresh()->access_token)->toBe('the-token-that-still-works'); + + // And it must not disconnect: the refresh_token is probably fine, so the + // next tick should retry rather than emailing the owner to reconnect. + expect($this->account->fresh()->status)->toBe(Status::Connected); + Queue::assertNotPushed(SendNotification::class); }); test('refreshToken reports false for a platform with nothing to refresh', function () { From f5d7d1f3d5ae90c089702e4411ba51840ce5ba2c Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 16:50:27 -0300 Subject: [PATCH 14/19] Cover the tokenless-200 guard on every platform, not just X MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard added in the previous commit protects nine refresh methods; only X had a test. Each provider reads a different field name out of the response, so a regression would land on one platform at a time and the suite would stay green for the other eight. The test drives every platform claiming a refresh flow through a 200 that carries no token, and requires each to refuse with PlatformUnavailableException — nothing is provably dead, so the next tick should retry rather than anyone being disconnected — while leaving the stored credential untouched. Verified it fails usefully by dropping the guard from one platform: 'threads should refuse a tokenless 200 cleanly, got QueryException: null value in column access_token violates not-null constraint'. Without naming the platform the failure reads as an unrelated database error, since the write also poisons the surrounding transaction. --- tests/Feature/Jobs/RefreshSocialTokenTest.php | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index b1015fa84..6d74ba84b 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -585,3 +585,49 @@ expect($checked)->toBeGreaterThan(0); }); + +test('no platform lets a tokenless 200 destroy the credential it already had', function () { + Queue::fake(); + + $verifier = app(ConnectionVerifier::class); + $checked = 0; + + foreach (Platform::cases() as $platform) { + if (! $platform->hasTokenRefreshFlow()) { + continue; + } + + $checked++; + + $account = SocialAccount::factory()->create([ + 'workspace_id' => Workspace::factory()->create()->id, + 'platform' => $platform, + 'status' => Status::Connected, + 'access_token' => 'the-token-that-still-works', + 'refresh_token' => 'rt-seed', + 'meta' => ['service' => 'https://bsky.social', 'identifier' => 'a.bsky.social'], + ]); + + // A 200 carrying no token at all. Every provider reads a different + // field name, so this is the shape none of them can parse. + Http::fake(['*' => Http::response(['expires_in' => 3600], 200)]); + + try { + $verifier->refreshToken($account); + $this->fail("{$platform->value} accepted a 200 with no token in it"); + } catch (PlatformUnavailableException) { + // Correct: nothing is provably dead, so refuse and let the next + // tick retry rather than disconnecting anyone. + } catch (Throwable $e) { + // Without the guard the write reaches the database and trips the + // NOT NULL column, which also poisons the surrounding transaction. + $this->fail("{$platform->value} should refuse a tokenless 200 cleanly, got ".$e::class.': '.$e->getMessage()); + } + + expect($account->fresh()->access_token) + ->toBe('the-token-that-still-works', "{$platform->value} overwrote a working token with nothing"); + } + + Queue::assertNotPushed(SendNotification::class); + expect($checked)->toBeGreaterThan(0); +}); From d87d76d9c890a81fea694e3188c395c0e8f92ed7 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 16:58:02 -0300 Subject: [PATCH 15/19] Stop the fallback from reading every failure as good news MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fifth review found the concurrent-refresh test passing without ever exercising what it claims to cover, and the reason it could is a real bug. access_token is an encrypted cast. The test wrote the winner's pair with DB::table()->update(), which stores plaintext, so reading it back raised DecryptException — and accessTokenStillWorks() caught Throwable and returned true. Green test, zero coverage of the recovery that justifies the method existing. It now writes through the model and asserts the verify call actually happened; removing the reload makes it fail. The catch is the bug. Treating any non-TokenExpiredException as "the token is healthy" means an APP_KEY rotation, a corrupted column, or an UnhandledMatchError from a newly added platform leaves the account Connected forever while every publish hard-fails, and nobody is told. It now names the outcomes that earn the benefit of the doubt — platform down, network dropped, account deleted mid-run — and lets the rest surface. Loud is right here: an APP_KEY rotation breaks every account at once, so failing the job where an operator sees it beats disconnecting every user. Refusing to persist a tokenless 200 also had no way out. The account kept retrying every 15 minutes forever, and the daily sweep counts PlatformUnavailableException as verified, so it was never disconnected and never reported. Now the retry only continues while there is a live token behind it: once that expires and renewal still fails, the connection is dead in practice and says so. Also corrects the refreshHttp() docblock, which claimed a blast radius the private method does not have — refreshToken() is what those 24 call sites reach — and records in VerifyWorkspaceConnections that short-TTL platforms never being re-verified is the intended consequence, not an oversight. --- app/Jobs/RefreshSocialToken.php | 20 +++++- app/Jobs/VerifyWorkspaceConnections.php | 14 +++- app/Services/Social/ConnectionVerifier.php | 15 +++-- tests/Feature/Jobs/RefreshSocialTokenTest.php | 65 ++++++++++++++++++- 4 files changed, 102 insertions(+), 12 deletions(-) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 182f6c02d..c637e1395 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -10,7 +10,9 @@ use App\Services\Social\ConnectionVerifier; use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Foundation\Queue\Queueable; +use Illuminate\Http\Client\ConnectionException; use Illuminate\Support\Facades\Log; use Throwable; @@ -57,6 +59,15 @@ public function handle(ConnectionVerifier $verifier): void 'platform' => $this->account->platform->value, 'error' => $e->getMessage(), ]); + + // Transient failures are retried on the next tick, but only while + // there is still a live token to fall back on. Once it has expired + // and we still cannot renew it, the connection is dead in practice + // — say so, rather than retrying in silence until the owner finds + // out from a failed post. + if ($this->account->is_token_expired) { + $this->account->markAsTokenExpired($e->getMessage()); + } } catch (TokenExpiredException $e) { if ($this->shouldTrustAWorkingAccessToken() && $this->accessTokenStillWorks($verifier)) { return; @@ -103,7 +114,14 @@ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool return $verifier->verifyAccessToken($this->account); } catch (TokenExpiredException) { return false; - } catch (Throwable $e) { + } catch (PlatformUnavailableException|ConnectionException|ModelNotFoundException $e) { + // Only genuinely transient outcomes get the benefit of the doubt: + // the platform being down, the network dropping, or the account + // being deleted out from under a job that has tries = 1. Anything + // else — a decrypt failure after an APP_KEY rotation, an + // UnhandledMatchError from a newly added platform — would otherwise + // read as "the token is healthy" and leave the account Connected + // forever while every publish hard-fails. Log::warning('Access token fallback check failed after a rejected refresh', [ 'account_id' => $this->account->id, 'platform' => $this->account->platform->value, diff --git a/app/Jobs/VerifyWorkspaceConnections.php b/app/Jobs/VerifyWorkspaceConnections.php index 815c367e1..fb1c16906 100644 --- a/app/Jobs/VerifyWorkspaceConnections.php +++ b/app/Jobs/VerifyWorkspaceConnections.php @@ -28,9 +28,17 @@ class VerifyWorkspaceConnections implements ShouldQueue // How long a recorded verification (SocialAccount::last_verified_at) is // trusted before this sweep re-checks the account. RefreshSocialToken - // stamps that field on every successful token refresh, and a refresh - // proves the credential just as well as the verify endpoint does — without - // the per-call charge providers like X bill for reading a profile. + // stamps that field on every successful token refresh, and a refresh does + // more than the verify endpoint does: it replaces the access token rather + // than inspecting it, so there is nothing left for a billed read to + // confirm. + // + // For short-TTL platforms this means the sweep never calls verify() again + // — X and Bluesky tokens live 2h and are refreshed ~90 minutes apart, so + // the stamp is never stale at the daily tick. That is intended, not an + // oversight: the refresh detects a revoked or dead credential 16× more + // often than this sweep did, for free. What it cannot see (a suspended + // account whose refresh still succeeds) surfaces at publish time. private const VERIFIED_WITHIN_HOURS = 12; public function __construct(public Workspace $workspace) {} diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index 79663b635..132136f45 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -29,12 +29,15 @@ class ConnectionVerifier /** * Read and connect timeouts for a token refresh. * - * Deliberately generous. refreshHttp() is shared with the ~24 publish and - * analytics call sites, and for X, Bluesky and TikTok the refresh_token is - * single-use: giving up on a request the provider has already processed - * loses the rotated pair for good and costs the user a manual reconnect. - * A token endpoint answers in milliseconds, so a long ceiling is nearly - * free, while a tight one turns provider slowness into dead accounts. + * These match the HTTP client's own defaults, and are stated here so a + * future change to those defaults cannot silently break the lock invariant + * below. They are deliberately generous: refreshToken() is reached from + * ~24 publish and analytics call sites as well as the scheduled job, and + * for X, Bluesky and TikTok the refresh_token is single-use, so abandoning + * a request the provider has already processed loses the rotated pair for + * good and costs the user a manual reconnect. A token endpoint answers in + * milliseconds, so a long ceiling is nearly free while a tight one turns + * provider slowness into dead accounts. */ public const REFRESH_TIMEOUT_SECONDS = 30; diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 6d74ba84b..c06deb05a 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -432,14 +432,19 @@ ]); // The winner persisted its new pair while ours was in flight; this - // instance still holds the rotated-away one. - DB::table('social_accounts')->where('id', $this->account->id)->update([ + // instance still holds the rotated-away one. Written through a separate + // model so the encrypted casts apply — a raw DB write stores plaintext, + // and reading it back throws DecryptException instead of exercising this. + SocialAccount::find($this->account->id)->update([ 'access_token' => 'winner-access-token', 'refresh_token' => 'winner-refresh-token', ]); (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + // The recovery is the point: reload, find the winner's token, verify with + // it. Asserting the call proves we got that far rather than bailing early. + Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me')); expect($this->account->fresh()->status)->toBe(Status::Connected); Queue::assertNotPushed(SendNotification::class); }); @@ -631,3 +636,59 @@ Queue::assertNotPushed(SendNotification::class); expect($checked)->toBeGreaterThan(0); }); + +test('a platform outage on an already-dead token stops being silent', function () { + Queue::fake(); + + $verifier = mock(ConnectionVerifier::class); + $verifier->shouldReceive('refreshToken')->once()->andThrow( + new PlatformUnavailableException('X API returned 503 during token refresh', 503) + ); + app()->instance(ConnectionVerifier::class, $verifier); + + // Already expired: there is no live token left to retry behind. + $this->account->update(['token_expires_at' => now()->subMinutes(5)]); + + (new RefreshSocialToken($this->account))->handle($verifier); + + expect($this->account->fresh()->status)->toBe(Status::TokenExpired); + Queue::assertPushed(SendNotification::class); +}); + +test('a platform outage on a live token stays quiet and retries', function () { + Queue::fake(); + + $verifier = mock(ConnectionVerifier::class); + $verifier->shouldReceive('refreshToken')->once()->andThrow( + new PlatformUnavailableException('X API returned 503 during token refresh', 503) + ); + app()->instance(ConnectionVerifier::class, $verifier); + + $this->account->update(['token_expires_at' => now()->addMinutes(20)]); + + (new RefreshSocialToken($this->account))->handle($verifier); + + expect($this->account->fresh()->status)->toBe(Status::Connected); + Queue::assertNotPushed(SendNotification::class); +}); + +test('a failure the fallback cannot attribute to the token surfaces instead of passing as healthy', function () { + Queue::fake(); + + $verifier = mock(ConnectionVerifier::class); + $verifier->shouldReceive('refreshToken')->once()->andThrow(new TokenExpiredException('rejected')); + // e.g. a decrypt failure after an APP_KEY rotation, or an unhandled match + // for a platform someone just added. + $verifier->shouldReceive('verifyAccessToken')->once()->andThrow(new RuntimeException('cannot decrypt')); + app()->instance(ConnectionVerifier::class, $verifier); + + // Swallowing this would leave the account Connected forever while every + // publish hard-fails. It has to reach failed_jobs where someone sees it — + // and it must not disconnect users, since an APP_KEY rotation breaks all + // of them at once. + expect(fn () => (new RefreshSocialToken($this->account))->handle($verifier)) + ->toThrow(RuntimeException::class); + + expect($this->account->fresh()->status)->toBe(Status::Connected); + Queue::assertNotPushed(SendNotification::class); +}); From 04f6d8479efffa19a9691613158f33882c8dfee5 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 17:13:33 -0300 Subject: [PATCH 16/19] Stop a bad hour at the provider from disconnecting anyone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The escalation added last round was wrong, and two neighbouring paths had the same shape of bug. Marking the account expired whenever a PlatformUnavailableException hit an already-expired token looked like it closed a silent-rot gap. But that exception is what TokenRefreshClient raises for 5xx, 429 and connection timeouts — so X rate-limiting for forty minutes around a two-hour token's expiry disconnected the account, emailed the owner, and hard-failed every scheduled post, with only the daily sweep to undo it. The rot it was meant to prevent surfaces at publish time anyway. Reverted: a transient failure never disconnects. refresh_token was left unguarded when access_token was hardened. data_get() only falls back when a key is absent, so a provider answering with an explicit "refresh_token": null wiped the stored one — and the next tick then threw "no refresh token available" without making a single call. Guarded in the same four places, falling back on blank rather than on missing. A held lock reported "nothing refreshed" even when the token was already dead, handing the caller a credential it knew was expired. The publisher posts with it, takes a 401, and PublishToSocialPlatform finalises the post as failed and disconnects the account — over a lock a dying worker left behind, for the two minutes it survives. It now says transient, which is what a refresh someone else is already running actually is. VerifyWorkspaceConnections promoted TokenExpired accounts back to Connected on verifyAccount()'s return value, which is also true for "could not check, don't disconnect". An unreachable platform therefore told owners their reconnect had worked when nothing was verified. Promotion moved next to the successful verify. Each of the four is pinned by a test, and each test was checked by reverting the fix and confirming it fails. --- app/Jobs/RefreshSocialToken.php | 9 ---- app/Jobs/VerifyWorkspaceConnections.php | 13 +++-- app/Services/Social/ConnectionVerifier.php | 31 ++++++++++-- tests/Feature/Jobs/RefreshSocialTokenTest.php | 48 +++++++++++++++++-- .../VerifyWorkspaceConnectionsTest.php | 20 ++++++++ 5 files changed, 97 insertions(+), 24 deletions(-) diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index c637e1395..f21e2e3ac 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -59,15 +59,6 @@ public function handle(ConnectionVerifier $verifier): void 'platform' => $this->account->platform->value, 'error' => $e->getMessage(), ]); - - // Transient failures are retried on the next tick, but only while - // there is still a live token to fall back on. Once it has expired - // and we still cannot renew it, the connection is dead in practice - // — say so, rather than retrying in silence until the owner finds - // out from a failed post. - if ($this->account->is_token_expired) { - $this->account->markAsTokenExpired($e->getMessage()); - } } catch (TokenExpiredException $e) { if ($this->shouldTrustAWorkingAccessToken() && $this->accessTokenStillWorks($verifier)) { return; diff --git a/app/Jobs/VerifyWorkspaceConnections.php b/app/Jobs/VerifyWorkspaceConnections.php index fb1c16906..f712442e7 100644 --- a/app/Jobs/VerifyWorkspaceConnections.php +++ b/app/Jobs/VerifyWorkspaceConnections.php @@ -62,11 +62,6 @@ public function handle(ConnectionVerifier $verifier): void } if ($this->verifyAccount($verifier, $account)) { - // If was TokenExpired but now verified OK, mark as connected again - if ($account->status === Status::TokenExpired) { - $account->markAsConnected(); - } - continue; } @@ -96,6 +91,14 @@ private function verifyAccount(ConnectionVerifier $verifier, SocialAccount $acco $verifier->verify($account); $account->update(['last_verified_at' => now()]); + // Promote here, not on this method's return value: it also returns + // true for "could not check, don't disconnect", and reviving an + // account on an outage tells the owner a reconnect worked when + // nothing was verified at all. + if ($account->status === Status::TokenExpired) { + $account->markAsConnected(); + } + return true; } catch (PlatformUnavailableException $e) { Log::warning('Social account verification skipped: platform unavailable', [ diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index 132136f45..79ef0b486 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -141,6 +141,16 @@ private function refreshThenVerify(SocialAccount $account, ?TokenExpiredExceptio * * @throws PlatformUnavailableException */ + private function rotatedTokenFrom(?array $data, string $key, string $current): string + { + $token = data_get($data, $key); + + // data_get() only falls back when the key is absent, so an explicit + // null overwrites. Providers that omit the field mean "keep using the + // one you have", and so does one that answers with nothing. + return blank($token) ? $current : (string) $token; + } + private function tokenFrom(?array $data, Platform $platform, string $key = 'access_token'): string { $token = data_get($data, $key); @@ -217,9 +227,20 @@ public function refreshToken(SocialAccount $account): bool $lock = Cache::lock("token_refresh:{$account->id}", self::REFRESH_LOCK_SECONDS); if (! $lock->get()) { - // Another process is already refreshing this token + // Another process is already refreshing this token. $account->refresh(); + if ($account->is_token_expired) { + // Reporting "nothing refreshed" would hand the caller a token + // it already knows is dead. A publisher posts with it, takes a + // 401, and PublishToSocialPlatform finalises the post as failed + // and disconnects the account — over a lock a dying worker left + // behind. Transient is the truth here: try again shortly. + throw new PlatformUnavailableException( + "A {$account->platform->label()} token refresh is already in progress." + ); + } + return false; } @@ -266,7 +287,7 @@ private function refreshLinkedInToken(SocialAccount $account): void $account->update([ 'access_token' => $this->tokenFrom($data, $account->platform), - 'refresh_token' => data_get($data, 'refresh_token', $account->refresh_token), + 'refresh_token' => $this->rotatedTokenFrom($data, 'refresh_token', $account->refresh_token), 'token_expires_at' => data_get($data, 'expires_in') ? now()->addSeconds(data_get($data, 'expires_in')) : null, ]); @@ -290,7 +311,7 @@ private function refreshXToken(SocialAccount $account): void $account->update([ 'access_token' => $this->tokenFrom($data, $account->platform), - 'refresh_token' => data_get($data, 'refresh_token', $account->refresh_token), + 'refresh_token' => $this->rotatedTokenFrom($data, 'refresh_token', $account->refresh_token), 'token_expires_at' => now()->addSeconds(data_get($data, 'expires_in', $account->platform->defaultTokenTtlSeconds())), ]); @@ -387,7 +408,7 @@ private function refreshTikTokToken(SocialAccount $account): void $account->update([ 'access_token' => $this->tokenFrom($data, $account->platform), - 'refresh_token' => data_get($data, 'refresh_token', $account->refresh_token), + 'refresh_token' => $this->rotatedTokenFrom($data, 'refresh_token', $account->refresh_token), 'token_expires_at' => data_get($data, 'expires_in') ? now()->addSeconds(data_get($data, 'expires_in')) : null, ]); @@ -414,7 +435,7 @@ private function refreshPinterestToken(SocialAccount $account): void $account->update([ 'access_token' => $this->tokenFrom($data, $account->platform), - 'refresh_token' => data_get($data, 'refresh_token', $account->refresh_token), + 'refresh_token' => $this->rotatedTokenFrom($data, 'refresh_token', $account->refresh_token), 'token_expires_at' => data_get($data, 'expires_in') ? now()->addSeconds(data_get($data, 'expires_in')) : null, ]); diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index c06deb05a..298e05f16 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -637,22 +637,25 @@ expect($checked)->toBeGreaterThan(0); }); -test('a platform outage on an already-dead token stops being silent', function () { +test('a platform outage never disconnects, not even once the token has expired', function () { Queue::fake(); $verifier = mock(ConnectionVerifier::class); $verifier->shouldReceive('refreshToken')->once()->andThrow( - new PlatformUnavailableException('X API returned 503 during token refresh', 503) + // TokenRefreshClient raises this for 5xx, 429 and connection timeouts. + new PlatformUnavailableException('X API returned 429 during token refresh', 429) ); app()->instance(ConnectionVerifier::class, $verifier); - // Already expired: there is no live token left to retry behind. $this->account->update(['token_expires_at' => now()->subMinutes(5)]); (new RefreshSocialToken($this->account))->handle($verifier); - expect($this->account->fresh()->status)->toBe(Status::TokenExpired); - Queue::assertPushed(SendNotification::class); + // A rate limit around expiry is not evidence of anything. Disconnecting + // here emails the owner and hard-fails every scheduled post, and only the + // daily sweep would undo it. + expect($this->account->fresh()->status)->toBe(Status::Connected); + Queue::assertNotPushed(SendNotification::class); }); test('a platform outage on a live token stays quiet and retries', function () { @@ -692,3 +695,38 @@ expect($this->account->fresh()->status)->toBe(Status::Connected); Queue::assertNotPushed(SendNotification::class); }); + +test('a null refresh_token in a 200 does not wipe the one we already had', function () { + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'access_token' => 'fresh-access-token', + 'refresh_token' => null, + 'expires_in' => 7200, + ], 200), + ]); + + $this->account->update([ + 'refresh_token' => 'the-refresh-token-that-still-works', + 'token_expires_at' => now()->addMinutes(20), + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + // data_get() only falls back when the key is absent, so an explicit null + // overwrites. Losing it means the next tick throws "no refresh token" + // without a single call, and the account dies with the access token. + expect($this->account->fresh()->refresh_token)->toBe('the-refresh-token-that-still-works'); +}); + +test('a refresh already in flight on a dead token is transient, not something to publish through', function () { + $this->account->update(['token_expires_at' => now()->subMinutes(5)]); + + Cache::lock("token_refresh:{$this->account->id}", 120)->get(); + + // Returning false here hands the caller a token it already knows is dead. + // A publisher then posts with it, gets a 401, and PublishToSocialPlatform + // finalises the post as failed and disconnects the account — for a lock + // that a worker death left behind. + expect(fn () => app(ConnectionVerifier::class)->refreshToken($this->account)) + ->toThrow(PlatformUnavailableException::class); +}); diff --git a/tests/Feature/VerifyWorkspaceConnectionsTest.php b/tests/Feature/VerifyWorkspaceConnectionsTest.php index 685e8a115..f59ac8e42 100644 --- a/tests/Feature/VerifyWorkspaceConnectionsTest.php +++ b/tests/Feature/VerifyWorkspaceConnectionsTest.php @@ -236,3 +236,23 @@ // on an account this sweep just confirmed healthy. expect($account->fresh()->last_verified_at)->not->toBeNull(); }); + +test('an unreachable platform does not revive an account nobody verified', function () { + Mail::fake(); + + $workspace = Workspace::factory()->create(); + $account = SocialAccount::factory()->x()->create([ + 'workspace_id' => $workspace->id, + 'status' => Status::TokenExpired, + ]); + + $verifier = mock(ConnectionVerifier::class); + $verifier->shouldReceive('verify')->andThrow(new PlatformUnavailableException('X API returned 503', 503)); + app()->instance(ConnectionVerifier::class, $verifier); + + VerifyWorkspaceConnections::dispatch($workspace); + + // "Don't disconnect" is not the same as "verified". Promoting on an + // outage tells the owner their reconnect worked when nothing was checked. + expect($account->fresh()->status)->toBe(Status::TokenExpired); +}); From 1195f341bd761d04b105a74e3d1f7260d028f2ad Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 17:31:32 -0300 Subject: [PATCH 17/19] Keep a lock collision off the analytics page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round seven found one real regression from round six, one consistency gap, and one stale comment. Reporting lock contention as transient was right for the publish path, which reschedules, but analytics calls refreshToken() bare and AnalyticsController has no try/catch — and there is no renderable handler for PlatformUnavailableException. A user opening analytics for an account whose token expired while the scheduled job held the lock got an HTTP 500 where the same request previously returned empty metrics. Reproduced before fixing: "Expected response status code [200] but received 500". The controller now degrades to empty numbers and still reports, which also covers the same 500 for any platform whose refresh 5xx'd — possible before this branch too. The fallback verify was throwing away a result worth keeping. It is a billed call on X and it proves the token alive exactly as a refresh does, so the pre-publish check was paying to ask the same question minutes later. Stamped like the other two sites. Also rewrites a comment in rotatedTokenFrom() that described the data_get() call it replaced rather than the blank() check beneath it. The review also reported a false @throws on that method; it has no docblock at all. Both fixes checked by reverting them and confirming the new tests fail. --- .../Controllers/App/AnalyticsController.php | 7 +++- app/Jobs/RefreshSocialToken.php | 11 +++++- app/Services/Social/ConnectionVerifier.php | 7 ++-- tests/Feature/AnalyticsResilienceTest.php | 37 +++++++++++++++++++ tests/Feature/Jobs/RefreshSocialTokenTest.php | 19 ++++++++++ 5 files changed, 75 insertions(+), 6 deletions(-) create mode 100644 tests/Feature/AnalyticsResilienceTest.php diff --git a/app/Http/Controllers/App/AnalyticsController.php b/app/Http/Controllers/App/AnalyticsController.php index cac42f7c6..008b2b2ae 100644 --- a/app/Http/Controllers/App/AnalyticsController.php +++ b/app/Http/Controllers/App/AnalyticsController.php @@ -72,7 +72,10 @@ public function show(Request $request, SocialAccount $account): JsonResponse $since = $request->has('since') ? Carbon::parse($request->input('since')) : null; $until = $request->has('until') ? Carbon::parse($request->input('until')) : null; - $metrics = match ($account->platform) { + // A transient platform failure — the provider being down, or a token + // refresh this request collided with — is not a server error. Empty + // numbers beat a 500 on a page the user just opened. + $metrics = rescue(fn () => match ($account->platform) { Platform::TikTok => app(TikTokAnalytics::class)->getMetrics($account), Platform::Instagram, Platform::InstagramFacebook => app(InstagramAnalytics::class)->getMetrics($account, $since, $until), Platform::Threads => app(ThreadsAnalytics::class)->getMetrics($account, $since, $until), @@ -83,7 +86,7 @@ public function show(Request $request, SocialAccount $account): JsonResponse Platform::YouTube => app(YouTubeAnalytics::class)->getMetrics($account, $since, $until), Platform::Telegram => app(TelegramAnalytics::class)->getMetrics($account), default => [], - }; + }, [], report: true); return response()->json(['metrics' => $metrics]); } diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index f21e2e3ac..2d16410ce 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -102,7 +102,16 @@ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool // has tries = 1, so an escaping ModelNotFoundException fails it. $this->account->refresh(); - return $verifier->verifyAccessToken($this->account); + if (! $verifier->verifyAccessToken($this->account)) { + return false; + } + + // This call is billed on X, and it proved the token alive just as + // well as a refresh would have. Record it so the pre-publish check + // does not pay to ask the same question minutes later. + $this->recordVerification(); + + return true; } catch (TokenExpiredException) { return false; } catch (PlatformUnavailableException|ConnectionException|ModelNotFoundException $e) { diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index 79ef0b486..55629dda7 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -145,9 +145,10 @@ private function rotatedTokenFrom(?array $data, string $key, string $current): s { $token = data_get($data, $key); - // data_get() only falls back when the key is absent, so an explicit - // null overwrites. Providers that omit the field mean "keep using the - // one you have", and so does one that answers with nothing. + // Blank, not just missing: data_get()'s own default would let an + // explicit null through and overwrite. A provider that omits the field + // means "keep using the one you have", and so does one that sends it + // empty. return blank($token) ? $current : (string) $token; } diff --git a/tests/Feature/AnalyticsResilienceTest.php b/tests/Feature/AnalyticsResilienceTest.php new file mode 100644 index 000000000..939766df4 --- /dev/null +++ b/tests/Feature/AnalyticsResilienceTest.php @@ -0,0 +1,37 @@ +create(); + $workspace = Workspace::factory()->create(['user_id' => $user->id]); + $user->update(['current_workspace_id' => $workspace->id]); + + $account = SocialAccount::factory()->x()->create([ + 'workspace_id' => $workspace->id, + 'status' => Status::Connected, + 'platform_user_id' => '4242', + // Expired, so the analytics service tries to refresh before reading. + 'token_expires_at' => now()->subMinutes(5), + ]); + + Http::fake(['*' => Http::response(['data' => [], 'meta' => []], 200)]); + + // The scheduled RefreshSocialToken is mid-refresh for this account. + Cache::lock("token_refresh:{$account->id}", 120)->get(); + + $response = $this->actingAs($user)->getJson(route('app.analytics.show', $account)); + + // A transient collision is not a server error. Before the lock started + // reporting itself as transient this returned empty metrics, and a 500 on + // a page the user just opened is a worse answer than no numbers. + $response->assertOk(); + expect($response->json('metrics'))->toBe([]); +}); diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index 298e05f16..373605f41 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -730,3 +730,22 @@ expect(fn () => app(ConnectionVerifier::class)->refreshToken($this->account)) ->toThrow(PlatformUnavailableException::class); }); + +test('a billed fallback check counts as a verification like any other', function () { + Http::fake([ + config('trypost.platforms.x.api').'/oauth2/token' => Http::response(['error' => 'invalid_grant'], 400), + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $this->account->update([ + 'access_token' => 'still-valid-access-token', + 'token_expires_at' => now()->addMinutes(20), + 'last_verified_at' => null, + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + // GET /2/users/me is billed and it just proved the token alive. Throwing + // that away means the pre-publish check pays to ask again minutes later. + expect($this->account->fresh()->last_verified_at)->not->toBeNull(); +}); From 20d2abaaf6d47ebde3add31d69e434e25844e80d Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 17:38:21 -0300 Subject: [PATCH 18/19] Degrade analytics on an unreachable platform, not on a bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rescue() added last commit caught Throwable, so it did not just absorb a platform being down — it absorbed everything. A TypeError in any metrics service rendered as "this account has no activity", with a log line as the only sign anything was wrong. Reproduced: a metrics service throwing RuntimeException returned 200 with empty metrics. This is the same mistake the review flagged two rounds ago in accessTokenStillWorks(), where treating any exception as "the token is healthy" hid real failures. Narrowed the same way: PlatformUnavailableException and ConnectionException degrade to empty numbers and still report, everything else surfaces as the 500 it is. The match moved into a named method so the intent has somewhere to live, since the reason for the narrow catch matters more than the catch itself. Both directions are pinned: widening the catch back to Throwable fails the bug-is-not-hidden test, and removing the degradation fails the lock-collision test. --- .../Controllers/App/AnalyticsController.php | 51 +++++++++++++------ tests/Feature/AnalyticsResilienceTest.php | 23 +++++++++ 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/app/Http/Controllers/App/AnalyticsController.php b/app/Http/Controllers/App/AnalyticsController.php index 008b2b2ae..608cff6ec 100644 --- a/app/Http/Controllers/App/AnalyticsController.php +++ b/app/Http/Controllers/App/AnalyticsController.php @@ -5,6 +5,7 @@ namespace App\Http\Controllers\App; use App\Enums\SocialAccount\Platform; +use App\Exceptions\PlatformUnavailableException; use App\Http\Controllers\Controller; use App\Models\SocialAccount; use App\Services\Social\FacebookAnalytics; @@ -16,6 +17,7 @@ use App\Services\Social\TikTokAnalytics; use App\Services\Social\XAnalytics; use App\Services\Social\YouTubeAnalytics; +use Illuminate\Http\Client\ConnectionException; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Support\Carbon; @@ -72,22 +74,41 @@ public function show(Request $request, SocialAccount $account): JsonResponse $since = $request->has('since') ? Carbon::parse($request->input('since')) : null; $until = $request->has('until') ? Carbon::parse($request->input('until')) : null; - // A transient platform failure — the provider being down, or a token - // refresh this request collided with — is not a server error. Empty - // numbers beat a 500 on a page the user just opened. - $metrics = rescue(fn () => match ($account->platform) { - Platform::TikTok => app(TikTokAnalytics::class)->getMetrics($account), - Platform::Instagram, Platform::InstagramFacebook => app(InstagramAnalytics::class)->getMetrics($account, $since, $until), - Platform::Threads => app(ThreadsAnalytics::class)->getMetrics($account, $since, $until), - Platform::Facebook => app(FacebookAnalytics::class)->getMetrics($account, $since, $until), - Platform::X => app(XAnalytics::class)->getMetrics($account, $since, $until), - Platform::LinkedInPage => app(LinkedInPageAnalytics::class)->getMetrics($account, $since, $until), - Platform::Pinterest => app(PinterestAnalytics::class)->getMetrics($account, $since, $until), - Platform::YouTube => app(YouTubeAnalytics::class)->getMetrics($account, $since, $until), - Platform::Telegram => app(TelegramAnalytics::class)->getMetrics($account), - default => [], - }, [], report: true); + $metrics = $this->metricsFor($account, $since, $until); return response()->json(['metrics' => $metrics]); } + + /** + * A platform being unreachable is not a server error: empty numbers beat a + * 500 on a page the user just opened, and a token refresh this request + * collided with resolves itself within seconds. + * + * Narrow on purpose. rescue() would have caught Throwable, so a defect in + * any metrics service would render as "this account has no activity" with + * nothing but a log line to say otherwise. + * + * @return array + */ + private function metricsFor(SocialAccount $account, ?Carbon $since, ?Carbon $until): array + { + try { + return match ($account->platform) { + Platform::TikTok => app(TikTokAnalytics::class)->getMetrics($account), + Platform::Instagram, Platform::InstagramFacebook => app(InstagramAnalytics::class)->getMetrics($account, $since, $until), + Platform::Threads => app(ThreadsAnalytics::class)->getMetrics($account, $since, $until), + Platform::Facebook => app(FacebookAnalytics::class)->getMetrics($account, $since, $until), + Platform::X => app(XAnalytics::class)->getMetrics($account, $since, $until), + Platform::LinkedInPage => app(LinkedInPageAnalytics::class)->getMetrics($account, $since, $until), + Platform::Pinterest => app(PinterestAnalytics::class)->getMetrics($account, $since, $until), + Platform::YouTube => app(YouTubeAnalytics::class)->getMetrics($account, $since, $until), + Platform::Telegram => app(TelegramAnalytics::class)->getMetrics($account), + default => [], + }; + } catch (PlatformUnavailableException|ConnectionException $e) { + report($e); + + return []; + } + } } diff --git a/tests/Feature/AnalyticsResilienceTest.php b/tests/Feature/AnalyticsResilienceTest.php index 939766df4..a1928d220 100644 --- a/tests/Feature/AnalyticsResilienceTest.php +++ b/tests/Feature/AnalyticsResilienceTest.php @@ -6,6 +6,7 @@ use App\Models\SocialAccount; use App\Models\User; use App\Models\Workspace; +use App\Services\Social\XAnalytics; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; @@ -35,3 +36,25 @@ $response->assertOk(); expect($response->json('metrics'))->toBe([]); }); + +test('a bug in a metrics service is not hidden behind empty numbers', function () { + $user = User::factory()->create(); + $workspace = Workspace::factory()->create(['user_id' => $user->id]); + $user->update(['current_workspace_id' => $workspace->id]); + + $account = SocialAccount::factory()->x()->create([ + 'workspace_id' => $workspace->id, + 'status' => Status::Connected, + 'token_expires_at' => now()->addHours(2), + ]); + + $this->mock(XAnalytics::class) + ->shouldReceive('getMetrics') + ->andThrow(new RuntimeException('a real bug, not the platform being down')); + + // Degrading to [] here would show the user an empty dashboard and leave a + // genuine defect looking like "this account has no activity". + $this->actingAs($user) + ->getJson(route('app.analytics.show', $account)) + ->assertStatus(500); +}); From e395d2e452c0d83a5edde9dadce92295d5114d2e Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 20 Aug 2026 17:44:29 -0300 Subject: [PATCH 19/19] Trim the commentary back to what the code cannot say MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RefreshSocialToken had 72 comment lines against 95 of code — 43% of the file. The rest of the branch was heading the same way: two constants in ConnectionVerifier carried twelve- and eight-line docblocks, and VERIFIED_WITHIN_HOURS had twelve lines explaining a number. Most of it was history rather than reasoning: what the code used to do, which review round asked for a change, the full argument for a decision the code already states. Kept the parts a reader cannot recover — why a catch is narrow, why a stamp is not written inside refreshToken(), why the lock has to outlast the timeouts — and cut the rest. shouldTrustAWorkingAccessToken() went with it: a one-line method behind a twelve-line docblock, now the condition it wrapped, inline where it is used. No behaviour change; full suite unchanged at 3786. --- .../Commands/RefreshExpiringTokens.php | 13 +-- .../Controllers/App/AnalyticsController.php | 10 +- app/Jobs/RefreshSocialToken.php | 93 +++++-------------- app/Jobs/VerifyWorkspaceConnections.php | 28 ++---- app/Services/Social/ConnectionVerifier.php | 73 +++++---------- app/Services/Social/XAnalytics.php | 16 +--- 6 files changed, 65 insertions(+), 168 deletions(-) diff --git a/app/Console/Commands/RefreshExpiringTokens.php b/app/Console/Commands/RefreshExpiringTokens.php index 0901c6578..5e4218a03 100644 --- a/app/Console/Commands/RefreshExpiringTokens.php +++ b/app/Console/Commands/RefreshExpiringTokens.php @@ -18,10 +18,9 @@ class RefreshExpiringTokens extends Command protected $description = 'Proactively refresh social tokens before they expire'; /** - * Rotating refresh_token platforms get a short lead: RefreshSocialToken - * rotates on every run, so a wider window would only rotate more often for - * no gain. Extension-model platforms (Instagram/Threads) can't be refreshed - * once expired, so they get a much wider lead to survive queue backlog. + * Rotating platforms get a short lead — a wider window would only rotate + * more often. Instagram and Threads can't be refreshed once expired, so + * theirs is wide enough to survive queue backlog. */ public function handle(): void { @@ -46,10 +45,8 @@ public function handle(): void } }); - // Accounts in the window, not jobs queued: RefreshSocialToken is unique - // per account, so a dispatch is discarded while another is still in - // flight — and a dispatched count would overstate itself during exactly - // the backlog an operator reads this line to diagnose. + // Accounts in the window, not jobs queued: the job is unique per + // account, so a dispatch during a backlog is silently discarded. $this->info("{$count} accounts due for a token refresh."); } } diff --git a/app/Http/Controllers/App/AnalyticsController.php b/app/Http/Controllers/App/AnalyticsController.php index 608cff6ec..8376afe8f 100644 --- a/app/Http/Controllers/App/AnalyticsController.php +++ b/app/Http/Controllers/App/AnalyticsController.php @@ -80,13 +80,9 @@ public function show(Request $request, SocialAccount $account): JsonResponse } /** - * A platform being unreachable is not a server error: empty numbers beat a - * 500 on a page the user just opened, and a token refresh this request - * collided with resolves itself within seconds. - * - * Narrow on purpose. rescue() would have caught Throwable, so a defect in - * any metrics service would render as "this account has no activity" with - * nothing but a log line to say otherwise. + * An unreachable platform is not a server error — empty numbers beat a 500 + * on a page the user just opened. Narrow on purpose: catching Throwable + * would render a defect as "this account has no activity". * * @return array */ diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 2d16410ce..c83a85736 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -22,11 +22,8 @@ class RefreshSocialToken implements ShouldBeUnique, ShouldQueue public int $tries = 1; - // Covers the full schedule cadence. RefreshExpiringTokens re-selects an - // account until its token_expires_at moves, which only happens once this - // job runs — so a backlogged queue would otherwise stack a job per tick, - // each rotating a single-use refresh_token again for nothing and widening - // the window where a worker death loses the pair. + // token_expires_at only moves once this job runs, so a backlogged queue + // would stack one job per tick for the same account. public int $uniqueFor = 900; public function __construct(public SocialAccount $account) {} @@ -37,15 +34,9 @@ public function uniqueId(): string } /** - * Refresh the token outright rather than verifying it first. - * - * A successful refresh already proves the credential is alive — the - * provider rejects a revoked one with a 4xx — so the verify endpoint adds - * nothing but cost. On X that endpoint is `GET /2/users/me`, billed as a - * "User: Read", and verifying a still-valid token left `token_expires_at` - * untouched: the account stayed inside RefreshExpiringTokens' window and - * was re-read every 15 minutes until the token actually died, which also - * left it expired for the stretch between expiry and the next tick. + * Refresh outright rather than verifying first: a refresh replaces the + * access token, leaving nothing for a verify call — billed as a "User: + * Read" on X — to confirm. */ public function handle(ConnectionVerifier $verifier): void { @@ -60,7 +51,12 @@ public function handle(ConnectionVerifier $verifier): void 'error' => $e->getMessage(), ]); } catch (TokenExpiredException $e) { - if ($this->shouldTrustAWorkingAccessToken() && $this->accessTokenStillWorks($verifier)) { + // Instagram and Threads extend their token in place and cannot + // renew it once expired, so a rejected extension means the + // connection is already doomed — say so while reconnecting still + // helps. Elsewhere a rejection often just means we lost a race. + if (! $this->account->platform->extendsAccessTokenOnRefresh() + && $this->accessTokenStillWorks($verifier)) { return; } @@ -75,53 +71,29 @@ public function handle(ConnectionVerifier $verifier): void } /** - * A rejected refresh does not on its own mean the connection is dead. - * X and Bluesky single-use their refresh_token — X issues a new one and - * invalidates the previous on every refresh, and refreshSession returns a - * mandatory new refreshJwt — so one a concurrent refresh already consumed - * is rejected while the current access_token keeps working. X is also - * documented by its own developer community to invalidate refresh_tokens - * spuriously. An account with no refresh_token at all fails here without - * any call being made at all. PublishToSocialPlatform hard-fails every post for a - * TokenExpired account, so disconnecting on a refresh rejection alone kills - * posts the access_token would still have published. - * - * This is the only place the (often billed) verify endpoint is reached from - * this job, and only after a refresh has already been rejected. A failure - * we can't attribute to the token — the platform being down, a network - * blip — leaves the account alone rather than disconnecting it on noise. + * The only place this job reaches the (billed) verify endpoint, and only + * once a refresh has been rejected — which on X and Bluesky usually means + * a concurrent refresh consumed the single-use refresh_token first. */ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool { - // A concurrent refresh may have persisted a new pair while ours was in - // flight, which is why ours was rejected. This instance still holds the - // token that was rotated away, so reload before judging it — otherwise - // the winner's healthy account gets disconnected. try { - // Inside the try: the account can be deleted mid-run, and this job - // has tries = 1, so an escaping ModelNotFoundException fails it. + // The winner of that race persisted a new pair; ours is stale. $this->account->refresh(); if (! $verifier->verifyAccessToken($this->account)) { return false; } - // This call is billed on X, and it proved the token alive just as - // well as a refresh would have. Record it so the pre-publish check - // does not pay to ask the same question minutes later. $this->recordVerification(); return true; } catch (TokenExpiredException) { return false; } catch (PlatformUnavailableException|ConnectionException|ModelNotFoundException $e) { - // Only genuinely transient outcomes get the benefit of the doubt: - // the platform being down, the network dropping, or the account - // being deleted out from under a job that has tries = 1. Anything - // else — a decrypt failure after an APP_KEY rotation, an - // UnhandledMatchError from a newly added platform — would otherwise - // read as "the token is healthy" and leave the account Connected - // forever while every publish hard-fails. + // Only these three earn the benefit of the doubt. Reading any other + // failure as "healthy" leaves the account Connected forever while + // every publish hard-fails. Log::warning('Access token fallback check failed after a rejected refresh', [ 'account_id' => $this->account->id, 'platform' => $this->account->platform->value, @@ -133,32 +105,9 @@ private function accessTokenStillWorks(ConnectionVerifier $verifier): bool } /** - * Whether a working access token is reason enough to stay connected after a - * refresh was rejected. - * - * It is for platforms that rotate a refresh_token, where a rejection often - * just means we lost a race and the current token is fine. It is not for - * Instagram and Threads: their long-lived token is extended in place and - * cannot be renewed once it expires, so a permanently rejected extension - * means the connection is already doomed. Staying connected because the - * token still reads would tell the owner only after it dies, when - * reconnecting is the only option left, and would keep retrying the - * rejected extension every 15 minutes across the whole 24-hour lead. - */ - private function shouldTrustAWorkingAccessToken(): bool - { - return ! $this->account->platform->extendsAccessTokenOnRefresh(); - } - - /** - * Record the refresh as a verification, so the daily sweep and the - * pre-publish check can skip their own (often billed) verify call. - * - * refreshToken() reports whether one actually ran, so a lock skipped by a - * concurrent refresh never reaches here. Deliberately not - * done inside ConnectionVerifier::refreshToken() — refreshThenVerify() - * calls it and can still fail on the verify that follows, and a stamp - * written there would vouch for a credential nothing ever confirmed. + * Lets the daily sweep and the pre-publish check skip a verify of their + * own. Not done inside refreshToken(): refreshThenVerify() calls that too + * and can still fail on the verify that follows. */ private function recordVerification(): void { diff --git a/app/Jobs/VerifyWorkspaceConnections.php b/app/Jobs/VerifyWorkspaceConnections.php index f712442e7..00973751a 100644 --- a/app/Jobs/VerifyWorkspaceConnections.php +++ b/app/Jobs/VerifyWorkspaceConnections.php @@ -26,19 +26,10 @@ class VerifyWorkspaceConnections implements ShouldQueue public int $timeout = 120; - // How long a recorded verification (SocialAccount::last_verified_at) is - // trusted before this sweep re-checks the account. RefreshSocialToken - // stamps that field on every successful token refresh, and a refresh does - // more than the verify endpoint does: it replaces the access token rather - // than inspecting it, so there is nothing left for a billed read to - // confirm. - // - // For short-TTL platforms this means the sweep never calls verify() again - // — X and Bluesky tokens live 2h and are refreshed ~90 minutes apart, so - // the stamp is never stale at the daily tick. That is intended, not an - // oversight: the refresh detects a revoked or dead credential 16× more - // often than this sweep did, for free. What it cannot see (a suspended - // account whose refresh still succeeds) surfaces at publish time. + // A refresh stamps last_verified_at and replaces the access token, so + // there is nothing left for a billed read to confirm. On short-TTL + // platforms the stamp is never stale here and this sweep stops calling + // verify() entirely — intended, not an oversight. private const VERIFIED_WITHIN_HOURS = 12; public function __construct(public Workspace $workspace) {} @@ -74,9 +65,8 @@ public function handle(ConnectionVerifier $verifier): void } /** - * Only a Connected account can be skipped. A TokenExpired one still needs - * the call: verifying it is how it gets promoted back to Connected, so - * trusting a stale stamp would strand a recovered account forever. + * Connected only: verifying a TokenExpired account is how it gets promoted + * back, so a stale stamp would strand one that has recovered. */ private function recentlyProvenValid(SocialAccount $account): bool { @@ -91,10 +81,8 @@ private function verifyAccount(ConnectionVerifier $verifier, SocialAccount $acco $verifier->verify($account); $account->update(['last_verified_at' => now()]); - // Promote here, not on this method's return value: it also returns - // true for "could not check, don't disconnect", and reviving an - // account on an outage tells the owner a reconnect worked when - // nothing was verified at all. + // Here, not on this method's return value — that is also true + // for "could not check, don't disconnect". if ($account->status === Status::TokenExpired) { $account->markAsConnected(); } diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index 55629dda7..7c38c91f0 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -27,17 +27,11 @@ class ConnectionVerifier { /** - * Read and connect timeouts for a token refresh. - * - * These match the HTTP client's own defaults, and are stated here so a - * future change to those defaults cannot silently break the lock invariant - * below. They are deliberately generous: refreshToken() is reached from - * ~24 publish and analytics call sites as well as the scheduled job, and - * for X, Bluesky and TikTok the refresh_token is single-use, so abandoning - * a request the provider has already processed loses the rotated pair for - * good and costs the user a manual reconnect. A token endpoint answers in - * milliseconds, so a long ceiling is nearly free while a tight one turns - * provider slowness into dead accounts. + * Read and connect timeouts for a token refresh. Stated explicitly, even + * though they match the client's defaults, so a change to those cannot + * silently break the lock invariant below. Generous on purpose: giving up + * on a request the provider already processed loses a single-use + * refresh_token for good. */ public const REFRESH_TIMEOUT_SECONDS = 30; @@ -46,12 +40,7 @@ class ConnectionVerifier /** * Must exceed the slowest refresh the timeouts above allow — Bluesky's two * sequential calls — or the lock lapses mid-flight and a second process - * refreshes with the same single-use refresh_token. Pinned by a test. - * - * The cost of erring long is that a worker dying mid-refresh leaves the - * lock held for this many seconds, during which a publish falls through to - * an expired token and retries. That is recoverable; an abandoned rotation - * is not. + * reuses the same single-use refresh_token. Pinned by a test. */ public const REFRESH_LOCK_SECONDS = 120; @@ -66,11 +55,9 @@ public function verify(SocialAccount $account): bool // Hard-expired tokens cannot make API calls — refresh is mandatory. // For tokens that are still valid OR only "expiring soon", try the // verify endpoint FIRST with the current access_token. This avoids - // rotating refresh_tokens unnecessarily — X and Bluesky invalidate the - // previous refresh_token on each refresh, so refreshing during races - // causes false-positive disconnects even though the access_token still - // works fine. (LinkedIn does not: it returns the same refresh_token, - // keeping the TTL from the original authorization.) + // rotating refresh_tokens unnecessarily — X and Bluesky single-use + // theirs, so refreshing during a race disconnects an account whose + // access_token still works. (LinkedIn returns the same token.) if ($account->is_token_expired) { return $this->refreshThenVerify($account); } @@ -130,12 +117,8 @@ private function refreshThenVerify(SocialAccount $account, ?TokenExpiredExceptio /** * Pull a token out of a refresh response, refusing to persist a blank one. * - * TokenRefreshClient classifies on HTTP status alone and never inspects the - * body, so a 200 carrying no token would otherwise be written straight over - * a credential that still works — and on Instagram and Threads, where the - * refresh_token is set to the same value, both halves go at once. Treat it - * as the platform misbehaving: the stored pair stays put and the next tick - * retries, instead of the account needing a manual reconnect. + * TokenRefreshClient classifies on HTTP status alone, so a 200 carrying no + * token would otherwise overwrite a credential that still works. * * @param array|null $data * @@ -145,10 +128,8 @@ private function rotatedTokenFrom(?array $data, string $key, string $current): s { $token = data_get($data, $key); - // Blank, not just missing: data_get()'s own default would let an - // explicit null through and overwrite. A provider that omits the field - // means "keep using the one you have", and so does one that sends it - // empty. + // Blank, not just missing: data_get()'s own default lets an explicit + // null through and overwrite. return blank($token) ? $current : (string) $token; } @@ -172,13 +153,9 @@ private function refreshHttp(): PendingRequest } /** - * Check the stored access token exactly as it is, skipping the - * refresh-and-retry ladder verify() runs. - * - * Callers that have just had a refresh rejected need this: routing through - * verify() would re-send the refresh_token the provider only just rejected, - * and on Bluesky re-run the password re-auth AT Proto rate-limits per - * account. + * Check the stored access token as it is, skipping the refresh-and-retry + * ladder verify() runs — which would re-send a refresh_token the provider + * just rejected, and on Bluesky re-run a rate-limited password re-auth. * * @throws TokenExpiredException if the access token itself is rejected * @throws PlatformUnavailableException if the platform is unreachable @@ -216,9 +193,8 @@ private function callVerifyEndpoint(SocialAccount $account): bool * use verify() instead. This method always attempts a refresh under * the per-account lock. * - * @return bool whether a refresh actually ran. False means another process - * already held the lock and this call did nothing, so callers - * must not treat it as having proven anything. + * @return bool whether a refresh actually ran — false means another + * process held the lock and this call proved nothing. * * @throws TokenExpiredException if refresh is rejected by the provider (4xx) * @throws PlatformUnavailableException if the platform is unreachable (5xx / network) @@ -232,11 +208,9 @@ public function refreshToken(SocialAccount $account): bool $account->refresh(); if ($account->is_token_expired) { - // Reporting "nothing refreshed" would hand the caller a token - // it already knows is dead. A publisher posts with it, takes a - // 401, and PublishToSocialPlatform finalises the post as failed - // and disconnects the account — over a lock a dying worker left - // behind. Transient is the truth here: try again shortly. + // Returning false would hand the caller a token it knows is + // dead; a publisher then posts with it, fails the post and + // disconnects the account. Transient is the truth here. throw new PlatformUnavailableException( "A {$account->platform->label()} token refresh is already in progress." ); @@ -247,9 +221,8 @@ public function refreshToken(SocialAccount $account): bool try { if (! $account->platform->hasTokenRefreshFlow()) { - // Facebook / InstagramFacebook use Page tokens that don't - // expire, Mastodon's don't either, and Telegram and Discord - // share one bot token with nothing per-account to refresh. + // Page tokens, Mastodon and the shared bot tokens have + // nothing per-account to refresh. return false; } diff --git a/app/Services/Social/XAnalytics.php b/app/Services/Social/XAnalytics.php index 6833a0882..dee5cc583 100644 --- a/app/Services/Social/XAnalytics.php +++ b/app/Services/Social/XAnalytics.php @@ -16,10 +16,7 @@ class XAnalytics { use HasSocialHttpClient; - /** - * Each page is billed per Post returned, so this bounds what one analytics - * load can cost as much as it bounds how long it takes. - */ + /** Each page is billed per Post returned, so this bounds cost as well as time. */ private const MAX_TIMELINE_PAGES = 5; private string $baseUrl; @@ -75,14 +72,11 @@ private function fetchMetricsFromApi(SocialAccount $account, CarbonInterface $si } /** - * Walk the account's timeline, summing each Post's public_metrics as the - * pages come back. + * Walk the timeline, summing public_metrics as the pages come back. * - * The metrics are requested from the timeline itself rather than looked up - * afterwards from /2/tweets. Both endpoints bill per Post returned, so - * re-reading the same ids only bought a second round-trip and a second - * claim on the same rate limit — the ids were already in hand, and their - * metrics come along for free on the request that fetched them. + * Asked for on the timeline request rather than looked up afterwards from + * /2/tweets: both bill per Post returned, so re-reading the same ids only + * bought a second round-trip. * * @return array{0: array, 1: int} totals, and how many Posts fed them */