Recovered `wpApiRestUrl` doesn't survive across launches on Atomic sites

[jkmassel]

#22903 lands a recovery flow that discovers and persists wpApiRestUrl for sites whose row landed in the DB with NULL. The persist works, but on Atomic / Jetpack-WPCom-REST sites the recovered value is clobbered back to NULL on every app foreground — the in-memory heal lives only for the current session.

Background

#22903 fixes the underlying gap (WP.com /me/sites omits the field; headless mint goes through the Jetpack tunnel and skips discovery). The recovery's SiteApiRestUrlRecoverer.persistApiRootUrl writes via SiteSqlUtils.updateWpApiRestUrl — a targeted single-column UPDATE. That part works correctly.

The remaining bug is that other FluxC writers do a full-row UPDATE via insertOrUpdateSite → WellSql.update().put(model, UpdateAllExceptId(...)), which writes every column including WP_API_REST_URL. If the in-memory SiteModel passed to that write has a stale (NULL) value for wpApiRestUrl, the full-row write clobbers whatever the recovery just persisted.

The race

Reported by @oguzkocer with a logcat reproduction on quick-six.jurassic.ninja (Atomic site, manually NULL'd the column, killed and relaunched):

T+0:     SiteAction-FETCH_SITES dispatched
T+679:   A_P: Hiding card for https://quick-six.jurassic.ninja - authenticated
T+1380:  Persisted wpApiRestUrl=https://quick-six.jurassic.ninja/wp-json/ for localId=9
T+1418:  Site found by (local) ID: 9
T+1420:  Updating site: https://quick-six.jurassic.ninja

After this sequence the DB column is NULL again.

What we tried in #22903

1. Preserve wpApiRestUrl from DB in createOrUpdateSites (reverted)

Hypothesis: the clobber came from FETCH_SITES → createOrUpdateSites (SiteStore.kt:1773). The existing preservation block in that function was gated on encrypted creds being present; moving wpApiRestUrl preservation outside the gate seemed like it'd plug the race.

Why it doesn't work: createOrUpdateSites receives sites from siteResponseToSiteModel (SiteRestClient.kt:1067), which constructs SiteModel() with id=0. insertOrUpdateSite's "Site found by (local) ID" log only fires when the inbound site's local id matches an existing row — it can't be 0. The handler that produced Oguz's "(local) ID: 9" log is somewhere else entirely.

2. Skip the persist for WPCom-REST sites in the slice (reverted)

Hypothesis: if we don't persist, we can't be clobbered. The slice's healApiRestUrlIfMissing would do an in-memory mutation only, and re-discover on next launch.

Why it doesn't work: the DB-level user-visible state is identical (NULL either way). The commit is honest about what it's doing — avoiding a pointless write — but it doesn't change the user-facing outcome and doesn't address the actual clobber.

Likely actual culprit

FETCH_SITE → SiteStore.updateSite (SiteStore.kt:1650-1677) is the strongest candidate. Two dispatches fire on every app foreground:

1. AppInitializer.kt:919 — updateSelectedSite.runIfNotLimited() → getSiteByLocalId(...) + newFetchSiteAction(site). The rate-limit is non-persistent so it always fires on relaunch.
2. WPMainActivity.java:1067 onResume → setSelectedSite → newFetchSiteAction.

SiteRestClient.fetchSite constructs a fresh SiteModel from the /sites/<id> response (which doesn't include wpApiRestUrl), explicitly stamps newSite.id = site.id, then returns. updateSite has the same buggy preservation gate as createOrUpdateSites — preserves wpApiRestUrl only when encrypted creds are present, and even when it does fire, the preservation is subject to the same TOCTOU between getSiteByLocalId and insertOrUpdateSite.

Not confirmed without further on-device instrumentation, but the log signature matches.

Other clobber sources worth investigating

• ReactNativeStore.persistSiteSafely — actively sets wpApiRestUrl = null on a 404 path from RN. High likelihood on Atomic sites since the block editor uses RN.
• CookieNonceAuthenticator — same 404 clobber pattern.
• SiteStore.designateMobileEditor, updateSiteEditors, updateApplicationPassword, designateMobileEditorForAllSites, onAllSitesMobileEditorChanged — each does a full-row insertOrUpdateSite write that can clobber if the in-memory site is stale.

Recommended fix direction

The structurally correct fix is to exclude WP_API_REST_URL from the UpdateAllExceptId mapper for SiteModel — either by extending UpdateAllExceptId (libs/fluxc/.../persistence/UpdateAllExceptId.java) to accept additional columns to skip, or by introducing a sibling mapper that excludes both _id and WP_API_REST_URL.

With this in place, SiteSqlUtils.updateWpApiRestUrl (the targeted single-column UPDATE introduced in #22903) becomes the sole writer for that column. Every other writer ignores it. No more clobbers, no per-handler preservation logic, no TOCTOU window.

The work:

• New / extended mapper (~25 lines).
• Route every insertOrUpdateSite UPDATE through it (already centralised in SiteSqlUtils.insertOrUpdateSite).
• Tests for the mapper and a regression test mimicking Oguz's race (concurrent updateWpApiRestUrl and a FETCH_SITE round-trip — verify the URL survives).

Scope: a few files, ~50 lines net. Deserves its own PR with focused review.

Related

• Populate missing wpApiRestUrl during editor preload #22903 — original recovery PR (in-memory heal ships; durable DB heal is this issue)
• Populate missing wpApiRestUrl during editor preload via WpLoginClient.apiDiscovery #22899 — the original "missing wpApiRestUrl" bug

[jkmassel]

Adversarial validation — actual primary culprit is different

Spent more time having multiple subagents try to prove and disprove each candidate. Two surprising results that change the picture.

SiteStore.updateSiteEditors is the actual primary foreground clobber

libs/fluxc/.../SiteStore.kt:2008-2022 — no preservation logic at all (unlike updateSite and createOrUpdateSites, which at least try). The handler sets mobileEditor/webEditor on the inbound site reference and writes it via insertOrUpdateSite without re-reading the DB.

The dispatch gate at AppInitializer.kt:272-275 is !AppPrefs.isDefaultAppWideEditorPreferenceSet(). I expected this to fail on aged installs (it's a v12.9→v13.0 migration check). It doesn't — the flag is effectively always-false:

• AppPrefs.isDefaultAppWideEditorPreferenceSet() (AppPrefs.java:730-733) returns !"".equals(getString(GUTENBERG_DEFAULT_FOR_NEW_POSTS)).
• Nothing writes that key in the modern codebase — only removeAppWideEditorPreference() (AppPrefs.java:743-745) and AppPrefs.reset(). Both deleters. Confirmed via grep.
• Missing key → getString returns "" → predicate is false → gate !false is true → dispatch fires.

So FETCH_SITE_EDITORS fires on every cold launch and every MySiteViewModel.onResume → SelectedSiteRepository.updateSiteSettingsIfNecessary cycle.

The clobber sequence:

• AppInitializer.kt:268 reads SiteModel via getSiteByLocalId(...) at T+0 — wpApiRestUrl=NULL because Oguz manually nulled it.
• Dispatches FETCH_SITE_EDITORS with that stale snapshot. SiteRestClient.fetchSiteEditors (SiteRestClient.kt:366-388) captures the same site reference in the response closure and round-trips it through FetchedEditorsPayload(site, ...).
• Recovery persists wpApiRestUrl at T+1380.
• FETCH_SITE_EDITORS network response arrives, updateSiteEditors writes the stale snapshot via insertOrUpdateSite → clobber.

SiteStore.updateSite — the original "strongest candidate" — actually preserves correctly for this reproduction

The gate !freshSiteFromDB.apiRestUsernameEncrypted.isNullOrEmpty() evaluates at handler time against a fresh DB read. For Oguz's reproduction:

• AP encrypted credentials are intact in DB (he only nulled wpApiRestUrl).
• Recovery has already persisted wpApiRestUrl to DB by the time the FETCH_SITE response arrives.
• Gate passes → wpApiRestUrl is copied from fresh DB row onto the inbound model → no clobber.

The trick I missed earlier: SiteSqlUtils.encryptAPIRestCredentials (SiteSqlUtils.kt:544-560) is called inside insertOrUpdateSite. When ApplicationPasswordLoginHelper stores plain creds with apiRestUsernameEncrypted = \"\", the persist step encrypts them. So after any successful AP login, the DB row's encrypted column is non-empty and the gate passes on subsequent reads. Same logic for createOrUpdateSites (SiteStore.kt:1772-1805).

updateSite and createOrUpdateSites ARE still latent clobbers — but only in narrow scenarios where encrypted AP creds are absent (post-clearApplicationPasswordColumns, or during a fresh-install AP-setup race). Not the reported scenario.

This also means the T+1418/T+1420 lines in Oguz's trace are most likely the recovery's own insertOrUpdateSite write (via updateApplicationPassword on the pre-#22903 branch, since that handler also produces those logs). The actual clobber happens later — when FETCH_SITE_EDITORS returns and updateSiteEditors writes back the stale snapshot.

ReactNativeStore.executeWithApplicationPassword 404 — disproved for Atomic

ReactNativeStore.kt:288-290 does null wpApiRestUrl on 404, but it's reachable only via executeWPAPIRequest. The routing fork in executeGetRequest/executePostRequest:

```kotlin
return@withDefaultContext if (site.isUsingWpComRestApi) {
executeWPComGetRequest(site, pathWithParams, enableCaching)
} else {
executeWPAPIGetRequest(site, pathWithParams, enableCaching)
}
```

For Atomic, SiteModel.isUsingWpComRestApi() returns true (isJetpackConnected() && origin == ORIGIN_WPCOM_REST, SiteModel.java:889-891) → always the WPCom branch → never reaches the AP path. This 404 clobber affects self-hosted WP-API / JPC-direct-host sites, not Atomic.

Other writers reviewed

• designateMobileEditor (SiteStore.kt:1964-1979) — user editor toggle, no preservation. Clobbers on user action, not foreground.
• designateMobileEditorForAllSites / onAllSitesMobileEditorChanged — fresh-from-DB reads minimize risk but TOCTOU-fragile.
• CookieNonceAuthenticator 404 (CookieNonceAuthenticator.kt:72-77) — self-hosted CookieNonce only.
• removeApplicationPassword (SiteStore.kt:1712-1734) — intentional clear, possibly too aggressive.
• updateSiteProfile — XML-RPC only, and newFetchProfileXmlRpcAction has no dispatcher in the app anyway.

Implications for the fix

The mapper-level fix (exclude WP_API_REST_URL from UpdateAllExceptId for SiteModel) is even more clearly the right approach:

• The actual primary clobber (updateSiteEditors) has no preservation to patch — fixing it handler-by-handler would mean retrofitting every writer.
• updateSite and createOrUpdateSites have buggy gates (preserving wpApiRestUrl only when encrypted AP creds are present, but the URL is orthogonal to AP encryption).
• The remaining writers are latent footguns; the next handler added without preservation will silently clobber.

Excluding WP_API_REST_URL at the mapper level makes SiteSqlUtils.updateWpApiRestUrl the sole writer of the column. No handler-level preservation, no TOCTOU windows, no gate condition complexity.

[jkmassel]

The same clobber applies to the application-password credential columns, and I think this issue's mapper fix should cover them in the same pass.

updateApplicationPassword (already in the "other clobber sources" list) persists the minted credentials via the same full-row insertOrUpdateSite → UpdateAllExceptId path. Any credential-less inbound SiteModel — e.g. a /me/sites sync model, which never carries the apiRest* fields — overwrites the encrypted columns with empty values. Identical mechanism to the wpApiRestUrl race, just on:

• API_REST_USERNAME (mApiRestUsernameEncrypted)
• API_REST_PASSWORD (mApiRestPasswordEncrypted)
• the two IV columns (mApiRestUsernameIV / mApiRestPasswordIV)

The four have to be excluded as a set — the IVs are required to decrypt the values, so protecting the ciphertext columns but not the IVs would still break reads.

Confirmed on-device

Surfaced while testing #22944 on a private Atomic site: the headless mint persisted the credentials, then ~1s of whole-row site writes during My Site load zeroed the columns, and the capability probe re-read empty creds and skipped its authenticated direct-host fallback → a false "Unable to connect to your site" banner. (#22944 works around it by handing the mint's credentials to the probe as an immutable value, but that's a point fix for a single reader.)

Suggested scope

Extend the UpdateAllExceptId exclusion set to WP_API_REST_URL + XMLRPC_URL + the four credential columns above, with the targeted single-column writers as the sole writers:

• updateWpApiRestUrl — exists (Populate missing wpApiRestUrl during editor preload #22903)
• updateXmlRpcUrl — added in Unify site provisioning + capability detection into one per-site source #22944
• a new targeted credential writer — updateApplicationPassword and the clears (clearApplicationPasswordColumns / removeApplicationPassword) switch to it

The payoff is broader than the banner: once the columns can't be clobbered, every direct reader of the credential columns is safe without per-reader repair logic — GutenbergKitSettingsBuilder (editor transport silently falls back to the WP.com proxy on empty creds), WpServiceProvider.createDelegate (require(...) → hard failure), JetpackConnectionHelper, the application-password client, and XML-RPC recovery.