Unify site provisioning + capability detection into one per-site source - #22944
Conversation
Generated by 🚫 Danger |
|
|
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22944 +/- ##
==========================================
+ Coverage 37.82% 37.91% +0.09%
==========================================
Files 2350 2352 +2
Lines 128587 128795 +208
Branches 17844 17898 +54
==========================================
+ Hits 48641 48836 +195
+ Misses 75965 75964 -1
- Partials 3981 3995 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
79c6afa to
50b2b88
Compare
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
2ad8edf to
85497da
Compare
Introduces EditorCapabilityDetector — one app-scoped owner of editor REST capability state, exposed as a per-site StateFlow — so the connectivity banner and editor preloader share a single, deduplicated probe instead of each re-deriving the state and racing the same async preconditions. Folds in the authenticated direct-host probe fallback so private Atomic sites detect correctly on trunk. Part of #22942.
#22926's first-login hardening signalled credential establishment through a process-global CredentialsChangedNotifier that the banner collected against a re-read of getSelectedSite() — the staleness race called out in #22942. EditorCapabilityDetector.refresh(storedSite), called from the mint path on the exact mutated SiteModel, replaces that coordination, so drop the notifier and its wiring in ApplicationPasswordLoginHelper.
…Source Promotes the EditorCapabilityDetector into SiteProvisioningSource: one per-site, single-flight pipeline that ensures application-password credentials, recovers the REST root, and detects editor capabilities — each stage awaited before the next. Because the capability probe is now structurally downstream of credential provisioning, it can never run before the mint, so the first-login race is gone by construction rather than mitigated. The application-password card, connectivity banner, and editor preloader all render slices of the one SiteReadiness state. The mint/validate mechanics move out of ApplicationPasswordViewModelSlice (now a renderer) into the source's ensureAuth stage; the per-site single-flight subsumes the card's old 409-safety guard. The duplicate wpApiRestUrl heal collapses into the source's recoverRestUrl stage.
Removes the threaded, mutated SiteModel from the provisioning pipeline: each stage now reads the site fresh by local id and writes back only the column it changed (persistApiRootUrl / a new persistXmlRpcUrl), so the parallel branches can't clobber one another and there's no stale-model write (#22905). With writes targeted, XML-RPC endpoint recovery moves out of the application-password card into the pipeline as a parallel branch off ensureAuth -- independent of the REST capability probe, since it needs only the credentials. The card becomes a pure renderer that reads the recovered xmlRpcUrl fresh. Adds SiteSqlUtils.updateXmlRpcUrl and a SiteXmlRpcUrlRecoverer mirroring SiteApiRestUrlRecoverer.
- @Suppress("ReturnCount") on ensureAuth — its five returns are each a distinct auth outcome; a single-return rewrite would read worse. - Drop a stray blank line before a brace left from removing a test region.
The IfNeeded suffix makes the short-circuit (skip when the field is already present / not applicable) clear at the call site.
WP.com Simple sites are proxy-served and OAuth-authed; the application-password mint returns NotSupported for them, so the pipeline was returning Unprovisionable and never reaching capability detection (which works fine through the proxy) -- a regression vs. the old ungated probe. ensureAuth now short-circuits them to a new SiteAuthState.NotApplicable (treated like Provisioned), and recoverRestUrlIfNeeded skips them too.
The route-support probe only sent Atomic sites to the direct host; Jetpack WPCom-REST sites fell through to the WP.com proxy. Since the proxy and the direct host advertise different route lists (the #22879 premise), Jetpack sites got the wrong answer. Broaden the predicate to isUsingWpComRestApi && !isWPComSimpleSite (Atomic + Jetpack); the proxy is only for minting the application password.
On a first provision of an Atomic site, the My Site screen showed a false "Unable to connect to your site" banner. `ensureAuth` minted an application password, but `detectCapabilities` re-read the `SiteModel` fresh and a concurrent whole-row site write (`insertOrUpdateSite` uses `UpdateAllExceptId`) had clobbered the just-encrypted credential columns before that read (#22905). With no credentials present, the authenticated direct-host probe was skipped, the unauthenticated discovery failed against the private host, and the probe reported the site unreachable. `ensureAuth` now returns the credentials it obtained in an internal `AuthResult`, and `detectCapabilities` overlays that immutable value onto its own coroutine-local `SiteModel` copy. The stages still read fresh and write only their own column — no `SiteModel` is shared or mutated across the parallel detect/`recoverXmlRpc` branches, so there's no race. Verified on-device against a private Atomic site: the authenticated direct-host probe runs and the banner is gone.
discoverAndVerifyXmlRpcUrl caught only DiscoveryException, so a RuntimeException from the discovery/verify path would escape the async, cancel the whole provisioning coroutine, and reach appScope (no SupervisorJob/handler). Catch CancellationException + generic Exception like the sibling SiteApiRestUrlRecoverer, so any failure degrades to the XML-RPC-disabled card and retries next run.
…riter #22947 made the app-password columns single-writer — excluded from the generic full-row update, written only by updateApplicationPasswordCredentials — so a fresh getSiteByLocalId after a mint reliably carries the credentials and a concurrent site write can't clobber them (#22905). The pipeline no longer needs to thread them as a value: remove ProvisionedCredentials, AuthResult.credentials, and the detectCapabilities overlay. ensureAuth returns a bare SiteAuthState; detectCapabilities and recoverXmlRpcIfNeeded read the credentials off the fresh read.
26b93d5 to
3f20dc4
Compare
SiteApiRestUrlRecoverer and SiteXmlRpcUrlRecoverer already contain their own throws, but launchPipeline runs the whole pipeline inside appScope.launch with no guard -- so any other escaping throw (a SQLiteException from a stage's WellSql write, a failure in detectCapabilities) still reaches appScope. appScope is a plain Job (no SupervisorJob/handler), so it cancels the scope, takes down every other app-scoped coroutine, and leaves the readiness flow stuck on Probing. Wrap the launch body like the recoverers: rethrow CancellationException so invalidate/clear stay clean, contain anything else as Unreachable so the flow settles and the next run retries. Adds a regression test that fails without the guard.
On a persistent 401, WPMainActivity and MediaBrowserActivity navigated straight to interactive re-auth, even for WP.com-connected sites whose application password can be re-minted headlessly -- and once a site latched Ready nothing re-validated its credentials, so a server-side revocation went unnoticed until pull-to-refresh. Route invalid-auth through the provisioning pipeline. SiteProvisioningSource listens to the raw wordpress-rs 401 (WpAppNotifierHandler) and re-runs ensureAuth: a WP.com-connected site re-mints silently; one that can't be settles Unprovisionable. Only that terminal failure escalates to interactive re-auth -- via a new app-scoped ApplicationPasswordReauthNotifier the two activities now observe instead of the raw 401. So a successful re-mint no longer flashes the re-auth screen, and bearer-only WP.com Simple sites no longer mis-trigger an application-password prompt. Churn guards: skip sites already Unprovisionable and WP.com Simple sites; the no-op-while-active invalidate keeps the heal's own validate-401 from looping. A 401-triggered run is flagged so only it, never a routine onResume run, can escalate to re-auth. Also documents why invalidate no-ops during an in-flight run.
…nPassword
Two review cleanups, no behavior change:
- GutenbergEditorPreloader re-read the provisioned site with
siteStore.sites.firstOrNull { it.id == siteId } -- a full-table read plus a
per-row credential decrypt to fetch one row by id. Use getSiteByLocalId(siteId),
the single-row lookup the rest of SiteProvisioningSource already uses.
- EditorSettingsRepository.isAwaitingApplicationPassword is dead: its only caller
(the connectivity banner's pending-auth suppression) was removed when detection
moved behind the pipeline. Delete it.
(SiteStore.persistXmlRpcUrl is also orphaned by this stack but lives in fluxc code
this PR doesn't touch -- left for a fluxc follow-up.)
- launchPipeline now contains the post-runPipeline tail. `maybeRequestReauth` reads the DB (`getSiteByLocalId`), and an escaping throw there would cancel the non-supervisor `appScope` and wedge provisioning for every site — wrap it so only `CancellationException` propagates. - `detectCapabilities` returns a private `PipelineResult(readiness, latch)`; the per-site dedup gate latches only on a live probe, so a `Ready` served from stale cache re-probes on the next run instead of sticking for the process lifetime. - A 401 that arrives while a run is in flight is deferred (`healForInvalidAuth`) instead of letting `invalidate` no-op and an unrelated run consume the re-auth flag — which could drop a revoked credential's interactive re-auth escalation. - `WpAppNotifierHandler.NotifierListener` hands listeners the `SiteModel`, not just the URL — URL is not unique (the constraint is `SITE_ID+URL`), so the heal now targets the exact row that 401'd. Drops the per-401 full site-table load+decrypt.
Three conflicts, all in the app-password area this branch reworked. MediaBrowserActivity: trunk swapped the CONNECTIVITY_ACTION receiver for LiveData<ConnectionStatus> while this branch swapped WpAppNotifierHandler for ApplicationPasswordReauthNotifier. Kept both. ApplicationPasswordViewModelSliceTest: took ours. Trunk's new tests all target slice methods this branch moved into SiteProvisioningSource. ApplicationPasswordViewModelSlice: trunk landed a fix in the block this branch deletes — the "XML-RPC Disabled" card now only shows on a *definitive* discovery negative, never on a transient failure (e.g. a 429), so throttled sites don't get a false warning. Carried that into the pipeline architecture rather than dropping it: discoverAndVerifyXmlRpcUrl returns XmlRpcRecovery (Recovered / Unavailable / Inconclusive), SiteProvisioningSource records definitive negatives and exposes isXmlRpcUnavailable, and the card is gated on that verdict. Also fixed the branch's SiteXmlRpcUrlRecovererTest stubs for the defaulted retryOnRateLimit param trunk added to SiteXMLRPCClient.fetchSites — it compiles either way but leaves Mockito a matcher short at runtime.
…re-auth Three defects in the new provisioning pipeline, found reviewing #22944. 1. The pipeline's own capability detection can raise the 401 that feeds back into healForInvalidAuth. getWpComApiClient installs the same invalid-auth notifier as the application-password client, so a rejected WP.com bearer token on an Atomic site 401s the theme fetch every run while the app password validates fine. Nothing broke the cycle: the run settles Ready or Unreachable, so neither the Unprovisionable guard nor the "already escalated" check fires, and the deferred heal relaunches forever. ensureAuth now reports whether it actually replaced the credentials, and PipelineResult carries whether auth was confirmed. A heal that confirmed a working password without replacing it cannot be the fix for a 401, so the site is recorded in healFutile and its 401s are dropped until an explicit retry. A heal that re-minted, or one that never reached the site, is not marked futile — so genuine revocation still heals. 2. The validator maps every ambiguous failure (DNS, timeout, refused, 5xx) to NetworkUnavailable so it never wipes credentials on a guess, and ensureAuth turned that into Provisioning. Since the banner renders only Unreachable and the card hides on Provisioning, a site that was simply down showed nothing at all — the exact case "Unable to connect to your site" exists for. ensureAuth now distinguishes device-offline from site-unreachable via SiteAuthState.SiteUnreachable, which maps to SiteReadiness.Unreachable. 3. A routine run never arms reauthOnFailure, so when a 401 arrived mid-run and that run settled Unprovisionable, the deferred handler returned "already escalated" when nothing had escalated. It now escalates instead of relaunching (a relaunch would only re-fail the mint), through a single escalateReauth funnel that is idempotent per Unprovisionable episode. Each fix has a test that fails when the fix is reverted; the loop test exhausts the test JVM without the guard. Also renames the transient-validation test, which was passing on a defaulted mock rather than a stated offline precondition.
Follow-up cleanup on the provisioning pipeline; no behaviour change. SiteAuthState had five variants but only two — Provisioning and Unprovisionable — were ever wrapped in NeedsAuth and observed by a consumer. The other three were ensureAuth control-flow signals that happened to live in a public sealed interface, which forced the application-password card into two dead `when` arms. ensureAuth now returns a private AuthStage (Proceed / SiteUnreachable / Stop), so runPipeline is a three-way when with no else and SiteAuthState means exactly what the card renders. PipelineResult carried credentialsChanged and authConfirmed, two booleans encoding a three-state fact with one combination that was unrepresentable in practice but perfectly constructible. They collapse into a HealEvidence enum (Inconclusive / ConfirmedUnchanged / Replaced) derived entirely in ensureAuth, so the futile-heal test is a single equality and runPipeline no longer sets authConfirmed on the side. This also drops a dead credentialsChanged pass-through that was always false on the NeedsAuth branch. No test file needed changing: nothing referenced the three removed variants. The loop test only passes when healFutile is populated, so it exercises the new evidence wiring end to end — verified by emitting Replaced on the Valid path, which reproduces the 401 loop and hangs the suite. One unreachable semantic shift: a WP.com Simple site used to satisfy the futile condition and now reports Inconclusive, which is the honest encoding since no application password applies. Simple sites return early from onRequestedWithInvalidAuthentication, so they can never be a heal.
The healFutile guard added a third early return to onRequestedWithInvalidAuthentication, one past detekt's limit of 2. The three skip conditions collapse cleanly into a conjunction, so extract them as canHeal() rather than suppressing the rule — the per-condition rationale reads better as KDoc on the predicate than as inline comments between guards. Also fixes a stale [maybeRequestReauth] KDoc link left by the rename to settleHealState. No behaviour change: canHeal reads the states map before the Simple-site check instead of after, which is a side-effect-free lookup.
The note was filed under 26.9, which shipped on 1 July. Trunk's top section is still labelled 27.0 even though versionName is 27.2 and both 27.0 and 27.1 have released — that top section is the accumulating bucket everything lands in, so append there rather than adding a new 27.2 heading. Left under 26.9 the note would never have reached a release.
Logging in to a private WordPress.com site reported "The provided site does not
support Application Password authentication." The site supports them fine — the
Privacy gate sits in front of WordPress and answers the anonymous discovery
request with 403 private_site, so discovery never reaches the REST API.
Two things were wrong.
The reason was thrown away. The ViewModel signalled failure by sending an empty
discoveryURL, and the fragment read that empty string as "unsupported" and
overwrote the specific message it had just been given. Every discovery failure
— DNS, timeout, TLS, 403, malformed URL — reported the same wrong cause. Fixed
structurally rather than by patching the message: discoveryURL now only ever
carries a URL to navigate to, and failure travels through errorMessage alone, so
no future failure path can clobber it either. setError() went with its only
caller.
The reason wasn't named. FailureFetchAndParseApiRoot carries a WpError with the
error code, message and status, so DiscoveryResult.Failed now carries a
FailureReason and the UI can explain a private site rather than guess. Verified
against a live private Atomic site rather than assumed: the library surfaces the
code as WpErrorCode.CustomException("private_site") with status 403.
Non-private failures now surface the library's own message, which is still
better than the previous blanket claim.
Leaves application_password_not_supported_error unreferenced. It is tagged
a8c-src-lib="module:login", so removing it means every values-*/strings.xml plus
whatever the string sync does — worth its own change.
Six review findings, four of them in the heal containment added earlier on this branch. The heal could disable itself on success. validate() talks through a notifier-wired client, so a revoked credential's 401 fires during the very heal that is fixing it. That scheduled a redundant second run, which re-validated the fresh password, recorded "confirmed unchanged", and permanently suppressed healing for the site. The deferred heal now stands down when the run it waited on already re-minted. Keying suppression on evidence type also missed the opposite case: a host that mints happily but never accepts the result produced Replaced every time, which never suppressed anything and re-minted on every 401 — accumulating application passwords on the user's account. healFutile is replaced by a spend counter: confirming an unchanged password exhausts the budget outright, a re-mint costs one, and reaching Ready without needing a heal restores it. invalidate() cleared that suppression below its in-flight guard, so it never ran for the sites that needed it — MySiteViewModel.refresh builds the application-password card first, and buildCard starts a run synchronously on Main.immediate. The clears move above the guard; only the relaunch has to respect an in-flight mint. escalateReauth read the DB and started an Activity while holding the class monitor, which the main thread takes on every stateFor / invalidate. The deferred handler now decides under the lock and acts outside it. Removing the fragment's blanket error overwrite took with it the only producer of application_password_not_supported_error — a site that advertises no application-passwords endpoint then showed raw untranslated exception text. That case gets its own FailureReason and the string back. The Jetpack install client lost buildUrl's empty-string guard when it moved off getWpApiClient, so an empty wpApiRestUrl reached ParsedUrl.parse instead of falling back. Tests use a validator stub that fires the notifier the way the real one does; plain thenReturn stubs hid the feedback loop that caused the first bug.
Three tests asserting each card emits the right SiteNavigationAction when tapped: the create and re-authentication cards open auto-authentication, the XML-RPC disabled card opens the bottom sheet. The slice's rendering was covered already; its click wiring was not.
…ilitystate-visibility
trunk c45e08e made the WP.com bearer client a no-op notifier, on the same reasoning the heal containment was written for: application-password reauthentication can't fix a bearer-token 401. That removes one of the two self-feeding 401 sources, so the comments no longer cite it. The containment still applies: getApplicationPasswordClient — the client validation uses — and the self-hosted capability probe both still report to the handler, which is what makes a heal schedule a redundant re-run of itself.
Revoke a private site's application passwords and pull to refresh, and the app said nothing at all. The pipeline was right — validate, wipe, mint, settle Unprovisionable — but building the re-authentication card needs an authorization URL, discovery can't get one through the Privacy gate, and the Failed branch posted null. The user is left with broken credentials and no explanation. The two discovery-failure branches (create card and re-auth banner) were identical, so they share one handler. It names the one cause this branch can recognise; everything else still hides pending #22884. Its own string rather than the login screen's: there the user is signing in, here they are already signed in and the site can't be reconnected.
The message sat off-centre and wrapped onto two lines because the layout constrains the text to start after the icon, and the card was passing ic_notice_white_24dp — a white icon on a white card. Nothing was drawn, but the space was still reserved. SingleActionCard gains a centerText flag: the image is hidden and the text spans the whole card. Reclaiming that space also fits the message on one line. The other four call sites take the default and are unaffected. Both branches of each layout decision are written out because view holders are recycled — a centred card must not leave the next one centred. bind() split into bindText / bindImage to stay under detekt's LongMethod limit.
The three tests asserting each card emits the right SiteNavigationAction covered wiring this PR never touches: neither OpenApplicationPasswordAutoAuthentication nor OpenXmlRpcDisabledBottomSheet appears as a changed line in the slice's diff, and they replaced nothing — the pre-PR test file had no click assertions at all. Good tests, wrong PR. This one is already flagged oversized, so they can land separately against the code they actually cover. Audited the rest and kept it: every other added test either belongs to a new class, replaces one removed in the same rewrite, or covers behaviour this PR changed.
…mpts Three review findings. SiteXmlRpcUrlRecoverer ran verifyOrDiscoverXMLRPCEndpoint — a blocking chain of HTTP calls that can take tens of seconds against a dead host — on BG_THREAD, which is Dispatchers.Default. That is the same CPU-sized pool APPLICATION_SCOPE runs every site's pipeline on, so a few slow self-hosted sites could park most of it. The code this replaced used IO_THREAD; restore that. escalateReauth marked the once-per-episode flag before knowing whether anyone took the prompt. Activities register their listener in onResume, so a heal that settles while the app is backgrounded notified an empty map and lost the prompt for good: later 401s are blocked by the Unprovisionable state, and routine runs settle with wasHeal = false. notifyReauthRequired now reports whether a live listener took it, and an undelivered prompt is held and retried by the next run — in practice the onResume run, once an activity has registered. The pipeline's catch-all mapped any throw to Unreachable without the offline check that ensureAuth and detectCapabilities both make, stacking the site banner on the global no-connection one. It now makes the same distinction. The first attempt at the second fix only stopped burning the flag, which left nothing to retry — the test caught it.
checkstyle's RegexpMultilineCheck runs over Kotlin too, and deleting the last test in the file left its trailing blank line sitting before the closing brace.
|
Testing look good. I just left a questions, and here is a point I do believe it is worth to have a look:
|
Reported on the PR: with "Unable to connect to your site" showing, turning the device offline leaves it stacked on the global "no connection" bar. Losing the network doesn't re-run the pipeline, so the readiness the banner collects stays Unreachable and nothing re-evaluates. The old code had the same hole — its suppressForOffline was computed once inside fetchCapabilities — but this PR added a second route into Unreachable (site unreachable at the auth stage), so the state is easier to be sitting in when the network drops. The banner now combines readiness with connectivity through a MediatorLiveData. ConnectionStatusLiveData only emits on transitions and swallows its initial value, so it serves as a change trigger and the decision reads isNetworkAvailable() for the live answer. Starting offline already behaved correctly — ensureAuth settles Provisioning, not Unreachable. This is only about losing the network while the banner is up.
Reported on the PR. The two recovery stages end in an unguarded WellSql update. Only the discovery half of XML-RPC recovery had a try/catch, so a throwing write — SQLiteException, database locked — escaped the async, cancelled the sibling through the enclosing coroutineScope, and settled the whole site Unreachable. The connectivity banner appeared and Ready never latched even though capability detection had already succeeded. The outer catch in launchPipeline anticipated a DB throw but contains it too coarsely: by then the successful result is gone. Both stages are best effort, so contain them individually and let the capability probe alone decide readiness. The report named the XML-RPC branch; persistApiRootUrl has the same hole, and worse placement — it runs before detectCapabilities in the capability branch, so a throw there kills the probe before it starts. Both are covered.
Nope, that's a regression. Fixed in 0f39bb1. untitled.webm |
Good catch - fixed in 3fba8a0. |



TL;DR
Getting a site ready — mint an application password, recover the REST root, recover XML-RPC, detect editor capabilities, heal a revoked credential — was spread across three consumers that each triggered their own slice and raced the others. One per-site, single-flight pipeline (
SiteProvisioningSource) now does it; the connectivity banner, application-password card, and editor preloader render slices of itsStateFlow<SiteReadiness>.PR 1 of #22942.
What this fixes
wpApiRestUrldoesn't survive across launches on Atomic sites #22905) — no stage holds aSiteModel; each reads fresh bysiteLocalIdand writes only its own column, backed by Make wpApiRestUrl, xmlRpcUrl, and app-password creds single-writer #22947's single-writer exclusions.CredentialsChangedNotifier— the event bus andgetSelectedSite()staleness race are gone.Unprovisionable: a 401-triggered run escalates to interactive re-auth, a routine run leaves it to the application-password card. Either way a successful re-mint no longer flashes the re-auth screen.Worth a reviewer's eye
Unreachablehas two producers: the capability probe failed, or the site was unreachable at the auth stage while the device was online. Both show the banner; the second is new (that state previously showed nothing).XmlRpcRecovery.Unavailable), not just a missingxmlRpcUrl— otherwise a 429'd site gets a false warning.Readywithout needing a heal restores it. A run that already re-minted stands its own deferred heal down — otherwise validation's 401 schedules a redundant re-run that suppresses healing after a successful heal.NotifierListener.onRequestedWithInvalidAuthenticationnow takesSiteModel, notsiteUrl— URL isn't unique (constraint isSITE_ID + URL).SiteProvisioningSourceis the only implementer.Out of scope
SiteStore.persistXmlRpcUrlis now orphaned — small fluxc cleanup.${site.url}/wp-json/. Only bites a non-standard REST root and failed auth.Testing
./gradlew :WordPress:testJetpackDebugUnitTest(provisioning source, XML-RPC recoverer, both slices, preloader, login helper + ViewModel, editor settings, reauth notifier) and./gradlew detekt.Site unreachable (new state)
Provisioning, notUnreachable.Application-password card
Credential revocation
A_P: Validating application password.Private-site login message