From b09f2472676656cb6bb41fae8910316e42c124a4 Mon Sep 17 00:00:00 2001 From: Tony Li Date: Tue, 4 Aug 2026 15:00:01 +1200 Subject: [PATCH 1/3] Make WordPress API cache bootstrap recovery safe and testable 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. --- Modules/Sources/WordPressCore/ApiCache.swift | 155 ++++++++++++++---- .../ApiCacheBootstrapTests.swift | 113 +++++++++++++ 2 files changed, 239 insertions(+), 29 deletions(-) create mode 100644 Modules/Tests/WordPressCoreTests/ApiCacheBootstrapTests.swift diff --git a/Modules/Sources/WordPressCore/ApiCache.swift b/Modules/Sources/WordPressCore/ApiCache.swift index c77da9ecdb51..148839d033d0 100644 --- a/Modules/Sources/WordPressCore/ApiCache.swift +++ b/Modules/Sources/WordPressCore/ApiCache.swift @@ -5,60 +5,157 @@ import WordPressApiCache import WordPressShared extension WordPressApiCache { - static func bootstrap() -> WordPressApiCache { - let instance: WordPressApiCache = .onDiskCache() ?? .memoryCache() - instance.startListeningForUpdates() - return instance + /// A failure encountered while opening the on-disk cache. It carries the + /// point of failure and the underlying error so the two can be reported + /// together for Sentry classification. + struct OnDiskCacheFailure: Error { + enum Kind: Equatable { + case couldNotOpenDatabase + case migrationFailed + case couldNotRemoveOrphanedSiblings + case couldNotDeleteDatabase + case couldNotRemoveSiblings + } + + let kind: Kind + let underlyingError: Error + } + + /// The result of opening the on-disk cache. + enum OnDiskCacheOutcome { + /// Opened an existing or freshly created cache with nothing to report. + case opened(WordPressApiCache) + /// Opened only after recovering from a broken database. The failure + /// that triggered recovery is surfaced for Sentry classification. + case recovered(WordPressApiCache, from: OnDiskCacheFailure) + /// Could not open an on-disk cache; the caller falls back to memory. + case failed(OnDiskCacheFailure) } - private static func onDiskCache() -> WordPressApiCache? { + static func bootstrap() -> WordPressApiCache { let cacheURL = URL.libraryDirectory.appending(path: "app.sqlite") - if let cache = WordPressApiCache.onDiskCache(file: cacheURL) { - return cache + let cache: WordPressApiCache + switch onDiskCache(at: cacheURL) { + case .opened(let opened): + cache = opened + case .recovered(let recovered, let failure): + report(failure) + cache = recovered + case .failed(let failure): + report(failure) + cache = memoryCache() } - if FileManager.default.fileExists(at: cacheURL) { - do { - try FileManager.default.removeItem(at: cacheURL) + cache.startListeningForUpdates() + return cache + } + + private static func report(_ failure: OnDiskCacheFailure) { + // Report each kind from its own call site. `wpAssertionFailure` derives + // its analytics identity and 7-day suppression key from #file/#line + // (see AssertionLogger), so a shared call site would let one kind's + // report suppress the others and collapse their Sentry grouping. + let userInfo = ["error": "\(failure.underlyingError)"] + switch failure.kind { + case .couldNotOpenDatabase: + wpAssertionFailure("Failed to create an instance", userInfo: userInfo) + case .migrationFailed: + wpAssertionFailure("Failed to migrate database", userInfo: userInfo) + case .couldNotRemoveOrphanedSiblings: + wpAssertionFailure("Failed to remove orphaned sqlite sibling files", userInfo: userInfo) + case .couldNotDeleteDatabase: + wpAssertionFailure("Failed to delete sqlite database", userInfo: userInfo) + case .couldNotRemoveSiblings: + wpAssertionFailure("Failed to remove sqlite sibling files", userInfo: userInfo) + } + } + + /// Opens (or creates) the on-disk cache at the given URL, recovering from a + /// broken database by deleting it and starting over. Internal so tests can + /// exercise the recovery path against a temporary location. + static func onDiskCache(at cacheURL: URL) -> OnDiskCacheOutcome { + let fileManager = FileManager.default - if let cache = WordPressApiCache.onDiskCache(file: cacheURL) { - return cache - } + // A previous app version may have deleted the database file without its + // sibling files. An orphaned journal left next to a freshly created + // database is a documented SQLite corruption vector, so remove any + // leftovers first and refuse to continue if that cleanup fails. + if !fileManager.fileExists(at: cacheURL) { + do { + try removeSiblingFiles(of: cacheURL) } catch { - wpAssertionFailure("Failed to delete sqlite database") + return .failed(OnDiskCacheFailure(kind: .couldNotRemoveOrphanedSiblings, underlyingError: error)) } } - return nil + switch openCache(file: cacheURL) { + case .success(let cache): + return .opened(cache) + case .failure(let failure): + // Only an existing database can be recovered by recreating it. + guard fileManager.fileExists(at: cacheURL) else { + return .failed(failure) + } + + do { + try fileManager.removeItem(at: cacheURL) + } catch { + return .failed(OnDiskCacheFailure(kind: .couldNotDeleteDatabase, underlyingError: error)) + } + + do { + // Deleting the database but not its journal is a documented + // SQLite corruption vector; always remove them together. + try removeSiblingFiles(of: cacheURL) + } catch { + return .failed(OnDiskCacheFailure(kind: .couldNotRemoveSiblings, underlyingError: error)) + } + + switch openCache(file: cacheURL) { + case .success(let cache): + return .recovered(cache, from: failure) + case .failure(let retryFailure): + return .failed(retryFailure) + } + } } - private static func onDiskCache(file: URL) -> WordPressApiCache? { + private static func openCache(file: URL) -> Result { let cache: WordPressApiCache do { cache = try WordPressApiCache(url: file) } catch { - wpAssertionFailure("Failed to create an instance") - return nil + return .failure(OnDiskCacheFailure(kind: .couldNotOpenDatabase, underlyingError: error)) } do { _ = try cache.performMigrations() } catch { - wpAssertionFailure("Failed to migrate database") - return nil + return .failure(OnDiskCacheFailure(kind: .migrationFailed, underlyingError: error)) } - do { - var url = file - var values = URLResourceValues() - values.isExcludedFromBackup = true - try url.setResourceValues(values) - } catch { - wpAssertionFailure("Failed exclude the database file from iCloud backup") - } + // Best-effort: keep the database out of iCloud backups. This can only + // fail in exotic filesystem states, and the cache is usable regardless, + // so it is not treated as a bootstrap failure. + var url = file + var values = URLResourceValues() + values.isExcludedFromBackup = true + try? url.setResourceValues(values) - return cache + return .success(cache) + } + + private static func removeSiblingFiles(of cacheURL: URL) throws { + let fileManager = FileManager.default + for suffix in ["-journal", "-wal", "-shm"] { + let sibling = URL(fileURLWithPath: cacheURL.path + suffix) + // Absent siblings are fine; only a genuine failure to remove one + // that exists is a corruption risk worth surfacing to the caller. + if fileManager.fileExists(at: sibling) { + try fileManager.removeItem(at: sibling) + } + } } private static func memoryCache() -> WordPressApiCache { diff --git a/Modules/Tests/WordPressCoreTests/ApiCacheBootstrapTests.swift b/Modules/Tests/WordPressCoreTests/ApiCacheBootstrapTests.swift new file mode 100644 index 000000000000..69f8c9e068ad --- /dev/null +++ b/Modules/Tests/WordPressCoreTests/ApiCacheBootstrapTests.swift @@ -0,0 +1,113 @@ +import Foundation +import Testing +import WordPressApiCache +@testable import WordPressCore + +@Suite +struct ApiCacheBootstrapTests { + private let directory: URL + + init() throws { + directory = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + } + + private var databaseURL: URL { + directory.appending(path: "app.sqlite") + } + + private var journalURL: URL { + URL(fileURLWithPath: databaseURL.path + "-journal") + } + + private var walURL: URL { + URL(fileURLWithPath: databaseURL.path + "-wal") + } + + private var shmURL: URL { + URL(fileURLWithPath: databaseURL.path + "-shm") + } + + @Test func createsDatabaseOnFirstBootstrap() { + let outcome = WordPressApiCache.onDiskCache(at: databaseURL) + + guard case .opened = outcome else { + Issue.record("Expected .opened, got \(outcome)") + return + } + #expect(FileManager.default.fileExists(at: databaseURL)) + } + + @Test func recoversFromCorruptDatabaseAndRemovesSiblings() throws { + try Data("not a database".utf8).write(to: databaseURL) + try Data("stale journal".utf8).write(to: journalURL) + try Data("stale wal".utf8).write(to: walURL) + try Data("stale shm".utf8).write(to: shmURL) + + let outcome = WordPressApiCache.onDiskCache(at: databaseURL) + + guard case .recovered(_, let failure) = outcome else { + Issue.record("Expected .recovered, got \(outcome)") + return + } + // The corrupt file may fail at open or at migration; either is recovery. + #expect(failure.kind == .couldNotOpenDatabase || failure.kind == .migrationFailed) + #expect(FileManager.default.fileExists(at: databaseURL)) + #expect(!FileManager.default.fileExists(at: journalURL)) + #expect(!FileManager.default.fileExists(at: walURL)) + #expect(!FileManager.default.fileExists(at: shmURL)) + } + + @Test func removesOrphanedSiblingsBeforeCreatingDatabase() throws { + // No database file, only leftovers a previous delete-on-failure may + // have stranded. SQLite ignores an unmatched -wal file in rollback + // journal mode, so only the pre-open cleanup removes it. + try Data("stale journal".utf8).write(to: journalURL) + try Data("stale wal".utf8).write(to: walURL) + + let outcome = WordPressApiCache.onDiskCache(at: databaseURL) + + guard case .opened = outcome else { + Issue.record("Expected .opened, got \(outcome)") + return + } + #expect(!FileManager.default.fileExists(at: journalURL)) + #expect(!FileManager.default.fileExists(at: walURL)) + } + + @Test func failsWhenOrphanedSiblingCannotBeRemoved() throws { + // A stale sibling next to a missing database that cannot be removed + // must not be paired with a freshly created database. Make the + // directory read-only so removing the journal fails. + try Data("stale journal".utf8).write(to: journalURL) + let fileManager = FileManager.default + try fileManager.setAttributes([.posixPermissions: 0o555], ofItemAtPath: directory.path) + defer { + try? fileManager.setAttributes([.posixPermissions: 0o755], ofItemAtPath: directory.path) + } + + let outcome = WordPressApiCache.onDiskCache(at: databaseURL) + + guard case .failed(let failure) = outcome else { + Issue.record("Expected .failed, got \(outcome)") + return + } + #expect(failure.kind == .couldNotRemoveOrphanedSiblings) + #expect(!fileManager.fileExists(at: databaseURL)) + } + + @Test func failsWhenDatabaseCannotBeCreated() { + let unwritableURL = + directory + .appending(path: "missing-subdirectory") + .appending(path: "app.sqlite") + + let outcome = WordPressApiCache.onDiskCache(at: unwritableURL) + + guard case .failed(let failure) = outcome else { + Issue.record("Expected .failed, got \(outcome)") + return + } + #expect(failure.kind == .couldNotOpenDatabase) + } +} From c1c29bff7817560a68bc7a43c963f775f5f6dd82 Mon Sep 17 00:00:00 2001 From: Tony Li Date: Tue, 4 Aug 2026 15:00:15 +1200 Subject: [PATCH 2/3] Share a single WordPressApiCache instance across all WordPressClients 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. --- Modules/Sources/WordPressCore/ApiCache.swift | 16 +++++++++++++++- .../Sources/WordPressCore/WordPressClient.swift | 10 +--------- .../ApiCacheBootstrapTests.swift | 11 +++++++++++ .../Posts/CustomPostEditorServiceTests.swift | 2 +- .../Posts/CustomPostSettingsViewModelTests.swift | 2 +- 5 files changed, 29 insertions(+), 12 deletions(-) diff --git a/Modules/Sources/WordPressCore/ApiCache.swift b/Modules/Sources/WordPressCore/ApiCache.swift index 148839d033d0..9035893b56dc 100644 --- a/Modules/Sources/WordPressCore/ApiCache.swift +++ b/Modules/Sources/WordPressCore/ApiCache.swift @@ -5,6 +5,16 @@ import WordPressApiCache import WordPressShared extension WordPressApiCache { + /// The one cache instance shared by every `WordPressClient`. + /// + /// A single SQLite database backs the cache for the whole app, so every + /// `WordPressApiCache` instance opens and migrates the same file. + /// Bootstrapping more than one concurrently can fail and destroy a + /// database still in use by another; sharing one instance keeps that to a + /// single bootstrap. Swift's once-only static initialization guarantees + /// it happens once per process. + public static let shared = WordPressApiCache.bootstrap() + /// A failure encountered while opening the on-disk cache. It carries the /// point of failure and the underlying error so the two can be reported /// together for Sentry classification. @@ -32,7 +42,7 @@ extension WordPressApiCache { case failed(OnDiskCacheFailure) } - static func bootstrap() -> WordPressApiCache { + private static func bootstrap() -> WordPressApiCache { let cacheURL = URL.libraryDirectory.appending(path: "app.sqlite") let cache: WordPressApiCache @@ -159,6 +169,10 @@ extension WordPressApiCache { } private static func memoryCache() -> WordPressApiCache { + // Fallback when the on-disk cache cannot be opened. Because the cache + // is a process-wide singleton, this degradation lasts the whole + // session by design; the cache is refetchable, so the cost is a cold + // cache until relaunch. // Creating an in-memory database should always succeed. let cache = try! WordPressApiCache() _ = try! cache.performMigrations() diff --git a/Modules/Sources/WordPressCore/WordPressClient.swift b/Modules/Sources/WordPressCore/WordPressClient.swift index f3c27ded44dd..6b481b45b844 100644 --- a/Modules/Sources/WordPressCore/WordPressClient.swift +++ b/Modules/Sources/WordPressCore/WordPressClient.swift @@ -81,16 +81,8 @@ public actor WordPressClient { /// The underlying API executor used for making network requests. public let api: any WordPressClientAPI - private var _cache: WordPressApiCache? public var cache: WordPressApiCache { - get { - if let _cache { - return _cache - } - let cache = WordPressApiCache.bootstrap() - _cache = cache - return cache - } + WordPressApiCache.shared } private var _service: WpService? diff --git a/Modules/Tests/WordPressCoreTests/ApiCacheBootstrapTests.swift b/Modules/Tests/WordPressCoreTests/ApiCacheBootstrapTests.swift index 69f8c9e068ad..3046cfd97296 100644 --- a/Modules/Tests/WordPressCoreTests/ApiCacheBootstrapTests.swift +++ b/Modules/Tests/WordPressCoreTests/ApiCacheBootstrapTests.swift @@ -110,4 +110,15 @@ struct ApiCacheBootstrapTests { } #expect(failure.kind == .couldNotOpenDatabase) } + + @Test func clientsShareSingleCacheInstance() async { + let clientA = WordPressClient(api: MockWordPressClientAPI(), siteURL: URL(string: "https://example.com")!) + let clientB = WordPressClient(api: MockWordPressClientAPI(), siteURL: URL(string: "https://example.org")!) + + let cacheA = await clientA.cache + let cacheB = await clientB.cache + + #expect(cacheA === cacheB) + #expect(cacheA === WordPressApiCache.shared) + } } diff --git a/Tests/KeystoneTests/Tests/Features/Posts/CustomPostEditorServiceTests.swift b/Tests/KeystoneTests/Tests/Features/Posts/CustomPostEditorServiceTests.swift index df54973fc4a4..def965629ca3 100644 --- a/Tests/KeystoneTests/Tests/Features/Posts/CustomPostEditorServiceTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Posts/CustomPostEditorServiceTests.swift @@ -276,7 +276,7 @@ private func makeService( api: api, siteURL: URL(string: "https://example.com")! ) - let wpService = try api.createService(cache: .bootstrap()) + let wpService = try api.createService(cache: .shared) return CustomPostEditorService( blog: blog, diff --git a/Tests/KeystoneTests/Tests/Features/Posts/CustomPostSettingsViewModelTests.swift b/Tests/KeystoneTests/Tests/Features/Posts/CustomPostSettingsViewModelTests.swift index 6d5841baf8b9..a80379b6acac 100644 --- a/Tests/KeystoneTests/Tests/Features/Posts/CustomPostSettingsViewModelTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Posts/CustomPostSettingsViewModelTests.swift @@ -309,7 +309,7 @@ private func makeServiceDependencies() throws -> (client: WordPressClient, wpSer api: api, siteURL: URL(string: "https://example.com")! ) - let wpService = try api.createService(cache: .bootstrap()) + let wpService = try api.createService(cache: .shared) return (client, wpService) } From b0d396677a4a21c3947b5d9dddf917795586419f Mon Sep 17 00:00:00 2001 From: Tony Li Date: Tue, 4 Aug 2026 15:57:53 +1200 Subject: [PATCH 3/3] Clean WordPress API cache during account and site removal 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. --- .../Networking/WordPressApiCacheCleanup.swift | 45 +++++++++++++++++++ .../Services/AccountService+Swift.swift | 4 ++ WordPress/Classes/Services/BlogService.m | 1 + 3 files changed, 50 insertions(+) create mode 100644 WordPress/Classes/Networking/WordPressApiCacheCleanup.swift diff --git a/WordPress/Classes/Networking/WordPressApiCacheCleanup.swift b/WordPress/Classes/Networking/WordPressApiCacheCleanup.swift new file mode 100644 index 000000000000..a71ee04b990d --- /dev/null +++ b/WordPress/Classes/Networking/WordPressApiCacheCleanup.swift @@ -0,0 +1,45 @@ +import Foundation +import WordPressAPI +import WordPressAPIInternal +import WordPressApiCache +import WordPressCore +import WordPressData + +extension BlogService { + @objc(removeWordPressApiCachedDataForBlog:) + public func removeWordPressApiCachedData(for blog: Blog) { + WordPressApiCache.shared.removeCachedData(for: blog) + } +} + +extension WordPressApiCache { + func removeCachedData(for blog: Blog) { + removeCachedData(for: [blog]) + } + + func removeCachedData(for blogs: Set) { + removeCachedData(for: Array(blogs)) + } + + private func removeCachedData(for blogs: [Blog]) { + for blog in blogs { + do { + // `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) + if isDirect { + try removeSelfHostedSite(url: blog.getUrl()) + } else if let siteID = blog.dotComID?.intValue { + try removeWordpressComSite(siteId: WpComSiteId(siteID)) + } else { + Loggers.app.error( + "Skipped WordPress API cache cleanup because the site's durable metadata is incomplete" + ) + } + } catch { + Loggers.app.error("Failed to remove WordPress API cached data: \(error)") + } + } + } +} diff --git a/WordPress/Classes/Services/AccountService+Swift.swift b/WordPress/Classes/Services/AccountService+Swift.swift index 57c9356f7672..24cd716b0675 100644 --- a/WordPress/Classes/Services/AccountService+Swift.swift +++ b/WordPress/Classes/Services/AccountService+Swift.swift @@ -3,6 +3,8 @@ import WordPressData import WordPressShared import ShareExtensionCore import WebKit +import WordPressApiCache +import WordPressCore extension AccountService { // MARK: - Current Account @@ -48,6 +50,8 @@ extension AccountService { return } + WordPressApiCache.shared.removeCachedData(for: account.blogs ?? []) + let objectID = TaggedManagedObjectID(account) coreDataStack.performAndSave { context in do { diff --git a/WordPress/Classes/Services/BlogService.m b/WordPress/Classes/Services/BlogService.m index 323d96c64d27..8090217959aa 100644 --- a/WordPress/Classes/Services/BlogService.m +++ b/WordPress/Classes/Services/BlogService.m @@ -344,6 +344,7 @@ - (void)removeBlog:(Blog *)blog DDLogInfo(@" remove", blog.displayURL); [blog.xmlrpcApi invalidateAndCancelTasks]; [self unscheduleBloggingRemindersFor:blog]; + [self removeWordPressApiCachedDataForBlog:blog]; WPAccount *account = blog.account;