RefreshMailOAuthTokensCommand is the daily refresh and, by its own docblock, the health check that goes with it: a delegated grant can die silently, and for a portal whose password-reset mails ride on this connection that must surface as a warning rather than as a support ticket weeks later. It decided whether to warn from last_error -- but last_error has a second writer. OAuthCodeFlowBroker::refresh() records a dead grant and notifies nobody, and freshAccessToken() reaches it from every send. So on an installation that is actually sending mail the send got there first, the command read the column as "already told them", and the warning never went out. last_error is cleared only by a successful refresh, which a dead grant never has, so it never went out later either. The alarm worked on installations that were not using the mailbox and failed on the ones that were.
The anti-nag rule is not the problem and does not change: one notification per broken state is still all anybody gets. The problem is that one column was answering two questions, which the table's own comment describes -- "what the settings page's warning and the admin notification read". The warning wants "is this connection broken", and any writer may answer it, which is why the settings page turning red on a failed send is correct and stays. The notification wants "have the admins been told", and only the notifier can answer that.
broken_notified_at is stamped when the command notifies, and the command asks that instead. It is cleared wherever last_error is cleared -- a successful refresh, a disconnect, a changed client id -- and those three sites now call clearFailure() rather than nulling two columns each, because a connection left healthy but still marked "already told them" would go quiet the next time it died, and a fourth caller is exactly how the first one happened. The send path still records the failure and still notifies nobody: a transport is not a place to decide who gets alarmed.
Verified before merging: 27 passed on the merged tree, 2 failed / 25 passed with app/ reset and the migration and tests kept. The recovery test is green either way by design. This touches the same command and broker as #1739 and the follow-up to it, so the merged result was read rather than trusted: the refresh reporting sits in the try and the notify guard in the catch, they do not interact, and refreshSerially() re-reads the row before refreshing so the broken_notified_at the catch reads is the stored one -- while a stand-aside throws nothing and never reaches the catch at all.
Note for the next release's upgrade notes: this adds a migration, so "nothing to do beyond dropping in the files" no longer holds.
Reported and fixed by @denkfabrik-li.
#1739 put the nightly OAuth refresh under the same lock a send holds, which is right -- but standing aside for the lock holder still printed "Refreshed <provider> (<account>)". No token request was made, so the line describes something that did not happen, and scheduler output is read precisely by somebody trying to work out what did.
refreshSerially() now answers whether it refreshed, and the command says which of the two happened. Standing aside is a healthy outcome: somebody else is refreshing this very connection, which slides the token window just as well as doing it again would. It is just not a refresh, and it should not claim to be one.
The existing test for the stand-aside now asserts the output too, and it fails against the old message.
Same reasoning as d8ef21b, which said when the worker check was skipped rather than skipping it quietly.
The daily refresh doubles as the health check for a connected OAuth
mailbox, and its own docblock says why that matters: a grant can die
silently, "which for a portal whose password-reset mails ride on this
connection must surface as a warning, not as a support ticket weeks
later".
It decided whether to warn by reading last_error -- but the send path
writes that column too. OAuthCodeFlowBroker::refresh() records the
failure and notifies nobody, and freshAccessToken() reaches it from every
send. So on an installation that actually sends mail, the send lands
first, the command reads the column as "already told them", and the
warning never goes out. last_error is cleared only by a successful
refresh, which a dead grant never has, so it never goes out again either.
Measured on main, one dead grant, two orders:
nobody sends, command first 1 notification, then quiet correct
a password-reset mail first 0 ... 0 ... 0 never
The alarm worked on installations that were not using the mailbox and
failed on the ones that were.
The anti-nag rule is not the problem and does not change. The problem is
that last_error answers "is this broken", which any writer may set, while
the command needs "have the admins been told", which only the notifier
can. The table's own comment shows the conflation -- one column described
as "what the settings page's warning and the admin notification read".
So the notification gets its own column. broken_notified_at is stamped
when the command notifies, and cleared wherever last_error is cleared: a
successful refresh, a disconnect, a changed client id. The three call
sites go through MailOAuthConnection::clearFailure() rather than nulling
two columns each, because a connection left marked "already told them"
while healthy would go quiet the next time it died -- the same bug in a
new place.
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.
Both fixes are follow-ups to the mail providers @denkfabrik-li added in #1679.
A refresh token is good for exactly one use — Microsoft and Google both
retire it as they issue the next one. Two queue workers finding the same
expired access token would therefore both spend it, and the loser gets
invalid_grant back. That is the same answer a revoked grant gives, so a
healthy connection would be marked broken, painted red on the settings
page and mailed to every admin. Refreshes now hold a per-connection lock
and whoever waits re-reads the row, which normally means finding a token
the winner already stored and not refreshing at all.
The Connect button read the provider dropdown, but the flow it starts
uses the saved provider. On an installation with both vendors registered,
switching without saving would open the wrong consent screen. The
dropdown now counts as an unsaved change like any other field, which also
gives it the right "save first" hint for free.
It is used by both OAuth mail test files, which --parallel runs in
separate processes — exactly the situation tests/Helpers.php exists
for, as its own header explains. CI caught what a whole-suite serial
run hides.
Adds "Microsoft 365 (OAuth)" to the Email settings provider dropdown.
Selecting it swaps the SMTP form for an app registration (client id,
secret, optional tenant) and a "Connect mailbox" flow: the admin signs
into the mailbox the installation should send as, and outgoing email
goes through Graph sendMail as that mailbox — no password, no app
password, no SMTP AUTH, which Microsoft is winding down.
Delegated flow on purpose: it needs no admin consent and works for
work/school and personal accounts alike. Its one weakness — a grant
can die silently behind a password reset or a Conditional Access
change — is answered by a daily scheduled refresh that keeps the
token alive and, on a dead grant, warns the settings admins once
in-app and on the settings page instead of letting mail stop quietly.
Tokens and the client secret live encrypted in their own row and are
read fresh at send time, never through the boot-config cache. The
stored SMTP transport survives a provider switch untouched.
ignacionelson