From 02eafb473b614d22f1b70348e478d44a4ecbb383 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Fri, 28 Aug 2026 04:19:05 +0200 Subject: [PATCH] Refresh a mailbox on the schedule under the lock a send would hold freshAccessToken() serialises refreshes per connection, and its comment says why: both providers rotate the refresh token as they hand out an access token, so the token is good for exactly one use, and "a worker racing the nightly refresh command means the slower one spends a token the faster one has already replaced. The provider answers that with invalid_grant, which is the same thing it says about a genuinely revoked grant: last_error gets written, the settings page turns red, and every admin is told to go and re-consent a connection that was never broken." The nightly refresh command called refresh() directly, outside that lock. It was the racer the comment names, not a party to the arrangement it describes. It now goes through refreshSerially(), which takes the same lock -- named once, in one place, for both callers -- re-reads the row inside it, and refreshes. Unlike freshAccessToken() it refreshes a token that is still usable, which is the point of the daily run: a delegated refresh token dies of disuse and this keeps the window sliding. The lock is taken rather than waited for, unlike the send path. Nobody is standing at a screen for a scheduled job, and a held lock means somebody is refreshing this very connection right now -- which slides the window and establishes its health just as well as doing it again would. One test: with the lock held, the command sends no token request and leaves the connection untouched. Without the fix it spends the refresh token the holder is already spending. --- .../Console/RefreshMailOAuthTokensCommand.php | 5 +- app/Modules/Platform/Mail/MailOAuthBroker.php | 9 +++ .../Platform/Mail/OAuthCodeFlowBroker.php | 55 ++++++++++++++++++- tests/Feature/Platform/MailOAuthTest.php | 34 ++++++++++++ 4 files changed, 101 insertions(+), 2 deletions(-) diff --git a/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php b/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php index 91976c5b..42382be2 100644 --- a/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php +++ b/app/Modules/Platform/Mail/Console/RefreshMailOAuthTokensCommand.php @@ -50,7 +50,10 @@ class RefreshMailOAuthTokensCommand extends Command $hadError = $connection->last_error !== null; try { - $brokers->for($connection->provider)->refresh($connection); + // Serialised against sends: refresh() on its own is the + // other half of the race freshAccessToken()'s lock is + // there to stop. + $brokers->for($connection->provider)->refreshSerially($connection); $this->info("Refreshed {$connection->provider->value} ({$connection->account_email})."); diff --git a/app/Modules/Platform/Mail/MailOAuthBroker.php b/app/Modules/Platform/Mail/MailOAuthBroker.php index 3958d2bc..cde14387 100644 --- a/app/Modules/Platform/Mail/MailOAuthBroker.php +++ b/app/Modules/Platform/Mail/MailOAuthBroker.php @@ -37,6 +37,15 @@ interface MailOAuthBroker */ public function refresh(MailOAuthConnection $connection): void; + /** + * A refresh that is not racing a send: the scheduled health check's + * way in, serialised against freshAccessToken() on the same + * connection. + * + * @throws MailOAuthException + */ + public function refreshSerially(MailOAuthConnection $connection): void; + /** * An access token currently valid for at least a small safety margin, * refreshing first when needed — what transports call at send time. diff --git a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php index 7a7b6d9b..cdfc2f54 100644 --- a/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php +++ b/app/Modules/Platform/Mail/OAuthCodeFlowBroker.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Modules\Platform\Mail; +use Illuminate\Contracts\Cache\Lock; use Illuminate\Contracts\Cache\LockTimeoutException; use Illuminate\Http\Client\Response; use Illuminate\Support\Facades\Cache; @@ -84,6 +85,48 @@ abstract class OAuthCodeFlowBroker implements MailOAuthBroker $this->storeTokens($connection, $response); } + /** + * The scheduled refresh, holding the same lock a send would. + * + * freshAccessToken() takes that lock because a refresh token is good + * for exactly one use, and it names this command as one of the racers: + * "a worker racing the nightly refresh command means the slower one + * spends a token the faster one has already replaced", which the + * provider answers with an invalid_grant indistinguishable from a + * revoked grant. The command was doing its refresh outside the lock, + * so it was the other half of that race rather than a party to it. + * + * Unlike freshAccessToken() this refreshes a token that is still + * usable, which is the point of the daily run: a delegated refresh + * token dies of disuse, and the refresh keeps the window sliding. + * + * Taken rather than waited for, unlike the send path: nobody is + * standing at a screen here, and a held lock means somebody is + * refreshing this very connection right now — which slides the window + * and establishes its health just as well as doing it again would. + * Spending the token behind them is the false alarm the lock exists to + * prevent. + */ + public function refreshSerially(MailOAuthConnection $connection): void + { + $lock = $this->refreshLock($connection); + + if (! $lock->get()) { + return; + } + + try { + // Re-read first: the winner may have stored tokens while this + // call was waiting, and refreshing the copy walked in with + // would spend a refresh token that is no longer current. + $connection->refresh(); + + $this->refresh($connection); + } finally { + $lock->release(); + } + } + public function freshAccessToken(MailOAuthConnection $connection): string { if ($this->stillUsable($connection)) { @@ -104,7 +147,7 @@ abstract class OAuthCodeFlowBroker implements MailOAuthBroker // re-read the row instead of trusting the copy it walked in with: by // the time the lock is theirs, the winner has already stored a token // they can just use. - $lock = Cache::lock('mail-oauth-refresh:'.$connection->provider->value, 30); + $lock = $this->refreshLock($connection); try { $lock->block(15); @@ -135,6 +178,16 @@ abstract class OAuthCodeFlowBroker implements MailOAuthBroker } /** Whether the stored access token has enough life left to send with. */ + /** + * One refresh at a time per connection, whoever is asking. The TTL + * outlives a token request and releases the claim if the holder dies + * mid-flight. + */ + private function refreshLock(MailOAuthConnection $connection): Lock + { + return Cache::lock('mail-oauth-refresh:'.$connection->provider->value, 30); + } + private function stillUsable(MailOAuthConnection $connection): bool { $token = $connection->access_token; diff --git a/tests/Feature/Platform/MailOAuthTest.php b/tests/Feature/Platform/MailOAuthTest.php index 6dc99de5..0811a8db 100644 --- a/tests/Feature/Platform/MailOAuthTest.php +++ b/tests/Feature/Platform/MailOAuthTest.php @@ -11,6 +11,7 @@ use App\Modules\Platform\Settings\MailConfigApplier; use App\Modules\Platform\Settings\MailProvider; use App\Modules\Platform\Settings\MailProviderSettings; use Illuminate\Support\Facades\Artisan; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Mail; @@ -425,6 +426,39 @@ test('the scheduled refresh keeps a healthy connection fresh', function () { ->and($connection->last_refreshed_at)->not->toBeNull(); }); +// freshAccessToken() takes a per-connection lock because a refresh token +// is good for one use, and its comment names this command as one of the +// racers. The command refreshed outside that lock, so it was the other +// half of the race rather than a party to it. +test('the scheduled refresh stands aside for a send that is already refreshing', function () { + Http::fake([ + 'login.microsoftonline.com/*' => Http::response(fakeTokenResponse([ + 'access_token' => 'access-token-2', + 'refresh_token' => 'refresh-token-2', + ])), + ]); + + connectMicrosoftMailbox(); + + $before = MailOAuthConnection::for(MailProvider::Microsoft365); + + // Somebody else is mid-refresh on this connection. + $held = Cache::lock('mail-oauth-refresh:microsoft365', 30); + expect($held->get())->toBeTrue(); + + Artisan::call('projectsend:refresh-mail-oauth-tokens'); + + // The token the holder is spending was not spent a second time. + Http::assertNothingSent(); + + $after = MailOAuthConnection::for(MailProvider::Microsoft365); + expect($after->access_token)->toBe($before->access_token) + ->and($after->refresh_token)->toBe($before->refresh_token) + ->and($after->last_error)->toBeNull(); + + $held->release(); +}); + test('a dead grant records the error and notifies settings admins exactly once', function () { Http::fake([ 'login.microsoftonline.com/*' => Http::response([