Clean WordPress API cache during account and site removal - #25864
Open
crazytonyli wants to merge 5 commits into
Open
Clean WordPress API cache during account and site removal #25864crazytonyli wants to merge 5 commits into
crazytonyli wants to merge 5 commits into
Conversation
Delete SQLite sibling files together with the database, clean up orphaned journal files before creating a new database, and model the result of opening the on-disk cache as an OnDiskCacheOutcome (opened, recovered, or failed) carrying a typed OnDiskCacheFailure. bootstrap() reports failures (each kind from its own call site so wpAssertionFailure's file/line-based analytics identity and suppression stay per-kind) and falls back to the in-memory cache, while the pure worker lets unit tests drive the recovery path without tripping assertionFailure in Debug. A failure to remove a sibling next to a fresh database is itself a corruption vector, so it fails rather than reopening against a stale journal.
Per-client cache instances each opened a connection to the same app.sqlite and re-ran migrations with BEGIN EXCLUSIVE, so concurrent bootstraps could fail with SQLITE_BUSY and delete a database other connections still had open (Sentry JETPACK-IOS-1KCJ). A process-wide shared instance bootstraps once, matching the Android app's singleton cache. Test helpers that built a service from bootstrap() now use the shared instance since bootstrap() is private.
Removing an account or site left its cached API data in app.sqlite, so a later re-add could read stale entries. Delete the cached data for each affected site during removal. The cleanup runs on the process-wide WordPressApiCache.shared instance rather than opening a separate connection to the database. A second connection would reintroduce the bootstrap contention this branch eliminates and would not notify the shared instance's update listeners, since SQLite update hooks fire only for writes on their own connection.
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 33708 | |
| Version | PR #25864 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 40b973d | |
| Installation URL | 4evpmekj47d4g |
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 33708 | |
| Version | PR #25864 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 40b973d | |
| Installation URL | 4ug5k7ffskf90 |
jkmassel
requested changes
Aug 11, 2026
| // `restApiRootURL` remains after an Atomic site's application | ||
| // password is removed, so it records that direct transport was | ||
| // configured without requiring the credential itself. | ||
| let isDirect = !blog.isHostedAtWPcom || (blog.isAtomic && blog.restApiRootURL != nil) |
Contributor
There was a problem hiding this comment.
Should this be a property on Blog? It seems like this could unintentionally diverge over time?
Contributor
Author
There was a problem hiding this comment.
I have re-written this part to remove this check: 270c379
| } | ||
|
|
||
| func removeDefaultWordPressComAccount() { | ||
| wpAssert(Thread.isMainThread, "Must be called from main thread") |
Contributor
There was a problem hiding this comment.
Do we want to run this cleanup job on the main thread?
Contributor
Author
There was a problem hiding this comment.
I think this assertion is less about "cleanup must happen in the main thread". It's more that the function uses Core Data instance that's bound to the main context, and it's only safe to call it from the main thread.
|
|
||
| private static func onDiskCache() -> WordPressApiCache? { | ||
| private static func bootstrap() -> WordPressApiCache { | ||
| let cacheURL = URL.libraryDirectory.appending(path: "app.sqlite") |
Contributor
There was a problem hiding this comment.
Might need to inject this one so we're not creating ~/Library/app.sqlite when running the test suite locally? The default could be URL.libraryDirectory though?
| } | ||
| } | ||
|
|
||
| extension WordPressApiCache { |
Contributor
There was a problem hiding this comment.
Should we have tests for these methods?
Contributor
Author
There was a problem hiding this comment.
Probably not absolutely necessary? The extension call the wordpress-rs APIs, which have unit test coverage.
Removing cached data inferred whether a site was stored under its self-hosted URL or its WordPress.com site ID, with a special case for Atomic sites. Instead, clear both keys the site has. Removal is a no-op when a key is absent, so trying both is safe, avoids leaving stale data behind when the classification would have been wrong, and drops a fragile heuristic.
The shared WordPressApiCache is a process-wide singleton backed by an on-disk database and reused across the whole test run, so writing through it would leak state between tests. Add forTesting(), which returns an isolated in-memory cache, and use it in the service test helpers that only need some cache. The bootstrap test that asserts the singleton identity keeps using shared.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This is an alternative of #25853. I recommend reviewing this PR commit by commit.
Description
The main changes is in
WordPressApiCache.Every
WordPressClientopened its ownWordPressApiCacheagainst the sameapp.sqliteand re-ran migrations. This PR makes the cache a single process-wide instance and to avoid unnecessary migration runs, which matches the Android app's singleton cache.The main changes:
OnDiskCacheOutcome(opened, recovered, or failed) carrying a typed failure.WordPressApiCache.sharedinstead of bootstrapping per client, so the database is opened and migrated once per process.