Skip to content

Record why a Bluesky login renewal failed - #234

Open
pfefferle wants to merge 12 commits into
trunkfrom
fix/session-refresh-resilience
Open

Record why a Bluesky login renewal failed#234
pfefferle wants to merge 12 commits into
trunkfrom
fix/session-refresh-resilience

Conversation

@pfefferle

@pfefferle pfefferle commented Aug 5, 2026

Copy link
Copy Markdown
Member

Context: https://wordpress.org/support/topic/push-single-post/#post-18984243

Two people lost their Bluesky connection, published a post that silently never went out, and asked how often they need to reconnect. I could not answer that, because when a login renewal fails the plugin keeps no record of it at all. mark_needs_reauth() stores that a reconnect is needed and throws away why, and there is nothing anywhere that says when the last renewal worked. Every one of these reports starts from zero.

This does not fix the disconnects. It makes them legible.

Proposed changes:

  • Client::refresh() now records every attempt in a new atmosphere_refresh_status option. A success stamps the time, written after the tokens land on disk, so the timestamp only ever means "the credentials really were renewed then".
  • A rejected refresh stores the auth server's own error code (invalid_grant, invalid_client, unauthorized_client) and logs it through debug_log(). Those three mean very different things: a consumed or replayed token, a client the auth server no longer knows, or a client barred from the grant.
  • A transport failure is recorded too. A site that cannot reach the auth server at all otherwise looks exactly like a site whose renewal worker never ran, and the advice for those two is opposite.
  • A failure never overwrites the last success. "Renewed fine for weeks, then got rejected" and "never renewed once" are different stories.
  • Site Health gets one new row, Last Login Renewal: never, or 2 hours ago, or 3 days ago (last failure 1 hour ago: invalid_grant). That is the line we can ask a reporter to paste.

The status lives in its own option rather than in atmosphere_connection on purpose. That row is written under the mid-flight disconnect and reconnect guards in refresh_locked(), and a failure record should not have to win those guards to be saved. It is cleared on disconnect and on a fresh authorization so a new session does not inherit the old one's history.

Other information:

  • Have you written new tests for your changes, if applicable?

14 new tests: the recording paths, races, and sanitizing in Test_Client_Refresh (11 of them), and the three debug-row shapes in Test_Health_Check.

Testing instructions:

You need a site connected to Bluesky.

  1. Go to Tools → Site Health → Info → ATmosphere. Last Login Renewal should read never on a site that has not renewed yet.
  2. Force a renewal: wp-env run cli wp cron event run atmosphere_refresh_token. Reload Site Health Info. The row should now say how long ago, in minutes.
  3. Simulate a rejection by corrupting the stored refresh token, then run the renewal again:
    wp-env run cli wp option patch update atmosphere_connection refresh_token nonsense
    The row should now show the last success plus a failure with an error code, and the connection should flip to needing a reconnect.
  4. Disconnect on the settings page. The row should go back to never, because the history belonged to the session that just ended.

Open questions

  • Should Site Health also warn when the renewal looks stalled, rather than only reporting it? I left that out for now. An hourly WP-Cron is plenty, since an expired access token is recovered inline on the next 401, so it is not obvious where the threshold for "stalled" should sit.
  • The harder problem is still untouched: Bluesky rotates the refresh token on every use, so a worker that dies after the server rotated but before our write lands leaves a dead session. The current design already refreshes as rarely as possible to shrink that window. Worth revisiting once we have real data from the row above.

@pfefferle pfefferle self-assigned this Aug 5, 2026
@github-actions github-actions Bot added [Feature] OAuth OAuth flow and authentication [Feature] WP Admin Admin UI and settings [Status] In Progress [Tests] Includes Tests PR includes test changes labels Aug 5, 2026
@pfefferle
pfefferle requested a review from a team August 31, 2026 11:13
@pfefferle
pfefferle marked this pull request as ready for review August 31, 2026 13:41
Copilot AI lite review requested due to automatic review settings August 31, 2026 13:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds diagnostics for OAuth login-renewal (refresh) attempts so support reports can distinguish “WP-Cron never ran” from “auth server rejected refresh” vs “transport failure,” and exposes that history in Site Health.

Changes:

  • Persist refresh-attempt outcomes in a new atmosphere_refresh_status option (timestamps + last error) and clear it on disconnect / fresh authorization.
  • Add a Site Health “Last Login Renewal” debug-info row that reports never, “X ago”, and optionally “last failure …: ”.
  • Add PHPUnit coverage for the recording logic and the new Site Health row, plus uninstall cleanup and a changelog entry.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
includes/oauth/class-client.php Records refresh successes/failures into a dedicated option; adds session/race guards and sanitization.
includes/wp-admin/class-health-check.php Adds “Last Login Renewal” Site Health debug-info field backed by the recorded refresh status.
tests/phpunit/tests/oauth/class-test-client-refresh.php Adds tests for refresh-status recording paths, races, and sanitization.
tests/phpunit/tests/wp-admin/class-test-health-check.php Adds tests for the new Site Health debug-info row output shapes.
uninstall.php Deletes the new refresh-status option on uninstall.
.github/changelog/fix-session-refresh-diagnostics Documents the end-user visible Site Health diagnostics addition.
Suppressed comments (1)

includes/wp-admin/class-health-check.php:630

  • Same timezone-offset issue as above: human_time_diff() should receive an explicit time() second argument so the “last failure … ago” value matches timestamps recorded with time().
			\__( '%1$s (last failure %2$s ago: %3$s)', 'atmosphere' ),
			$success,
			\human_time_diff( (int) $status['last_failure'] ),
			(string) ( $status['last_error'] ?? \__( 'unknown', 'atmosphere' ) )
		);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/phpunit/tests/wp-admin/class-test-health-check.php
Comment thread includes/wp-admin/class-health-check.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] OAuth OAuth flow and authentication [Feature] WP Admin Admin UI and settings [Status] In Progress [Tests] Includes Tests PR includes test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants