From 440538022511f235a9eed84170f59af1ea3070a4 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 1 Oct 2026 19:27:41 -0600 Subject: [PATCH] fix(ios): free editors mid-fetch, and share site requests in flight (#701) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ios): free editors mid-fetch, and share site requests in flight Follow-ups to keeping the fetch running, from reviewing #651: - The async dependency fetch no longer holds its editor. - A cancelled asset bundle build is never published. - Every cache for a site shares one SQLite store. - Identical requests and bundle builds in flight are shared. The fetch held its editor for as long as it ran: `await self?.prepareEditor()` optional-chains a weak `self` into an async call, which holds a strong `self` across every suspension inside it. A host that released the editor mid-fetch didn't free it until the fetch ended, and in between the full load tail — bundle provider, upload server bind, `loadFileURL` — still ran on a controller nobody held. The fetch now belongs to an `EditorDependencyLoader`, and the editor never awaits it. The editor owns the loader; the loader reaches back only through a `weak let delegate` whose requirements are all synchronous, so nothing it calls can suspend while holding the editor. A released editor is freed at once and nothing runs on it, while the fetch, still never cancelled, runs on and warms the cache for the next editor. The task starts from `fetch(from:)` rather than `init`, where a bare `delegate` would resolve to the strong parameter instead of the weak property. `prepareEditor()` goes away: the async flow is now "fetch, then the fast path", through `startLoadingEditor(dependencies:)`, which also takes over the #357 note about cancelling mid-`startUploadServer()`. The progress view now fades out as the load starts, rather than after `loadEditor` returns. `EditorAssetLibrary.buildBundle` published bundles from a cancelled build. Its task group swallows every per-asset failure, cancellation included, so a cancelled build reached `bundle.copy(to:)` with assets missing — and `readAssetBundles()` reads only the manifest, so every later launch served the gap. It now checks for cancellation before publishing. WordPress-iOS's `EditorDependencyManager._invalidate` can reach this today: it cancels an in-flight prefetch and purges without waiting for the task to finish. Every `EditorService` builds its own `EditorURLCache`, and each opened its own `SQLiteKVCache` on the site's `editorurlcache.sqlite` — which the store documents as undefined behavior, and measured, it is worse than contention. `connection()` opens lazily and caches the result, failure included, for the life of the instance, and nothing set a busy timeout. Two caches making their first read at the same moment left at least one of them broken in 50 runs out of 50, every later read and store throwing `databaseUnavailable`. Opened one after the other and then written concurrently, 189 of 400 writes still failed; through one instance, none did. That is the shape of WordPress-iOS's launch — `warmUpEditor(for:)` starts the warmup editor's fetch and the prefetch together, each with its own service — and a broken cache fails `prepare()` outright, since a read error is not a network error. Not reproduced in WordPress-iOS itself. `SQLiteKVCache.shared(handle:directory:diskCapacity:)` now hands every caller the live instance for its file, held weakly so a file no one is using is closed as before, and asserts that callers sharing a file ask for the same capacity. Being weak, it can hand out a fresh instance while the last one's `deinit` is still checkpointing the WAL, so the store now also sets a 5s busy timeout: reopening in that window failed 200 times in 200 without it, and never with it. The timeout doesn't replace `shared`. With it set, two instances opening at once still break one, because the switch to WAL returns `SQLITE_BUSY` without waiting on it. Nothing site-level was shared while in flight, so an editor opened mid-prefetch repeated the prefetch's requests and its bundle build, splitting the bandwidth the prefetch needed. Sharing now happens at the level of what goes over the wire and what lands on disk, which needs no analysis of the editor configuration: - `EditorHTTPClient.perform(_:)` joins an identical request already in flight. The key is the request as configured — URL, method, and headers, auth included — plus the session, and the timeout and network service type, which `URLRequest`'s own `==` ignores (measured). Only safe requests without a body are shared, and only from clients no delegate is watching. The table is process-wide, and since `EditorHTTPClient` is public, that includes a host's own GETs. - `EditorAssetLibrary.buildBundle(for:)` joins a build in flight for the same directory: storage root and manifest checksum. That holds whatever client either library has. The build downloads over the client of the library that started it, but the bundle is shared by site once it's on disk anyway, and builds kept apart by client race into the same directory through `copy(to:)`, where one can fail. Both go through `InFlightTasks`: cancelling a caller ends only that caller's wait, and shared work stops once no caller is left waiting on it. A caller that joins raises the work to its own priority, since waiting on a continuation doesn't escalate it the way awaiting `task.value` would; that needs iOS 26 or macOS 26. An editor opened mid-prefetch now joins the settings, theme, site settings, post types, and bundle build already in flight. It still fetches its own post, and the `editor-assets` manifest: that isn't cached, and the prefetch's request for it has usually finished by the time its build is running. The shared store is what makes this safe: a shared response reaches every waiter at the same instant, and each writes it through its own `EditorURLCache`. A shared task reports progress to its waiters one at a time, and a waiter can leave while an earlier one's callback is suspended. So `InFlightTasks` checks each waiter is still waiting just before its turn, and `EditorService.incrementProgress` drops progress that arrives after its `prepare()` has cleared it, rather than trapping on a precondition. Neither is enough alone: a call already under way when its caller leaves can't be recalled. With only the old precondition, a shared build reporting to a service whose `prepare()` had given up trapped, reproduced with two services sharing a build. The guard also fixes an older trap: two overlapping `prepare()` calls on one service, where the first to finish clears progress the second is still reporting. `theInFlightFetchKeepsTheEditorAlive` flips to `releasingTheEditorMidFetchFreesIt`: against the previous commit the editor is still alive after 2s; it now passes in 0.36s. Each of these fails against the code it pins: `buildBundlePublishesNothingWhenCancelled` with the cancelled bundle on disk, both new `EditorURLCacheTests` with one store per cache, `sharedReopensAFileWhileItCloses` without the busy timeout, `aCallerThatHasLeftHearsNoMoreProgress` without the re-check, `overlappingPrepareCallsDontTrap` without the guard, and `aHigherPriorityCallerRaisesTheTask` without the escalation. Mutation-tested too: a loader holding its delegate across the `await`, never sharing requests, and keying builds per library rather than per directory are each caught. `ParkedURLSession` moves to `Helpers/` so these suites can share it. * fix(ios): don't share a failed cache store, or the request for a post Follow-ups from review of the sharing this branch introduces. `SQLiteKVCache.shared` went on handing out an instance that could no longer work, to every caller for as long as anything held it: - One whose open failed. An instance keeps that failure for life, so a passing fault — a full disk, an I/O error — failed every later `prepare()` for the site, where each service used to get its own attempt. A failed instance now takes itself out of the registry. It has closed its handle, so the next caller's instance has the file to itself. `shared` doesn't ask the instance whether it failed: that would wait on `openLock`, held for the whole open and so for as long as the busy timeout, on the main thread where an editor builds its service. - One whose file `EditorViewController.deleteAllData()` had deleted. Both `get` and `put` through it throw "disk I/O error (code 10)", so the next editor failed to load rather than starting from an empty cache. `deleteAllData()` now goes through `EditorURLCache.deleteAll(in:)`, which stops sharing every store under the directory it removes. The stale instance closing later leaves the new file alone: 20 entries of 20 written beside it survived. `RESTAPIRepository.fetchPost` went through the shared `perform(_:)`, so an editor reopened on a post joined the GET an editor since closed still had in flight for it — a response that can predate an edit made in between. The post is deliberately never cached, and is now never shared either: `perform(_:)` sends a request alone when its cache policy asks to skip the cache, and the post request asks. A host's own requests through `EditorHTTPClient` can opt out the same way. `buildBundlePublishesNothingWhenCancelled` checked the disk as soon as its caller's wait ended, and `InFlightTasks` ends that wait before it cancels the build. With `try Task.checkCancellation()` removed the test still passed in 100 runs of 100 run one at a time, the bundle landing on disk moments later. It now waits for the abandoned build through a `task(for:)` test hook, and fails against that mutant in 20 runs of 20. `deliversTheError` gains the `defer` that `ParkedURLSession.release()` asks of every test. Each new test fails without its fix: a failed instance left in the registry, `deleteAll` not forgetting its stores, the forgotten prefix matching a sibling directory, the client ignoring the cache policy, and the post request keeping the default one. --- docs/code/preloading.md | 26 +- .../Sources/EditorHTTPClient.swift | 70 +++++- .../Sources/EditorViewController.swift | 92 +++---- .../Sources/Helpers/InFlightTasks.swift | 166 +++++++++++++ .../Sources/RESTAPIRepository.swift | 5 +- .../Services/EditorDependencyLoader.swift | 50 ++++ .../Sources/Services/EditorService.swift | 13 +- .../Sources/Stores/EditorAssetLibrary.swift | 24 +- .../Sources/Stores/EditorURLCache.swift | 26 +- .../Sources/Stores/SQLiteKVCache.swift | 93 ++++++- .../EditorHTTPClientTests.swift | 78 ++++++ .../EditorViewControllerLifecycleTests.swift | 90 +------ .../Helpers/ParkedURLSession.swift | 77 ++++++ .../InFlightTasksTests.swift | 228 ++++++++++++++++++ .../EditorDependencyLoaderTests.swift | 96 ++++++++ .../Services/EditorServiceTests.swift | 66 +++++ .../Services/RESTAPIRepositoryTests.swift | 23 ++ .../Stores/EditorAssetLibraryTests.swift | 62 +++++ .../Stores/EditorURLCacheTests.swift | 62 +++++ .../Stores/SQLiteKVCacheTests.swift | 110 +++++++++ ios/Tests/GutenbergKitTests/TestHelpers.swift | 15 ++ 21 files changed, 1333 insertions(+), 139 deletions(-) create mode 100644 ios/Sources/GutenbergKit/Sources/Helpers/InFlightTasks.swift create mode 100644 ios/Sources/GutenbergKit/Sources/Services/EditorDependencyLoader.swift create mode 100644 ios/Tests/GutenbergKitTests/Helpers/ParkedURLSession.swift create mode 100644 ios/Tests/GutenbergKitTests/InFlightTasksTests.swift create mode 100644 ios/Tests/GutenbergKitTests/Services/EditorDependencyLoaderTests.swift diff --git a/docs/code/preloading.md b/docs/code/preloading.md index e00795a56..7912f3365 100644 --- a/docs/code/preloading.md +++ b/docs/code/preloading.md @@ -313,6 +313,28 @@ let dependencies = try await service.prepare { progress in } ``` +#### Sharing Work Between Services + +Every `EditorService` for a site reads and writes the same on-disk caches, so there's no need to hand a service from a +prefetch to the editor — create one for each caller. Don't call `prepare()` on a service while an earlier call on it is +still running: progress is tracked per service, so the later call takes over the progress callback, and whichever +finishes first stops progress for both. + +Services for the same site also share work while it's in flight. A request identical to one already in flight joins it +rather than going out again, and a build of an asset bundle joins the one already running. So an editor opened before a +prefetch finishes fetches only its own post and the `editor-assets` manifest, even when the two are for different posts. +Requests are shared only between clients with the same `URLSession` instance, credentials, and timeout, and never from a +client with a delegate, which expects to see every request it makes. A bundle build is shared by every service for the +site whatever its client, just as the bundle it produces is once it's on disk. + +The request for the post is never shared, even between two editors on the same post: one already in flight can predate +an edit made since. It opts out through its cache policy — a request that asks to skip the cache +(`.reloadIgnoringLocalCacheData` and its siblings) always goes out on its own — and a host's own requests through +`EditorHTTPClient` can do the same. + +Cancelling a caller ends only that caller's wait; shared work stops once no caller is left waiting on it. `purge()` +doesn't stop it, so work that began before a purge can still land after it. + ### EditorViewController Loading Flows `EditorViewController` supports two loading flows based on whether dependencies are provided: @@ -337,7 +359,9 @@ let editor = EditorViewController( ) ``` -The editor displays a progress bar while fetching, then loads once complete. +The editor displays a progress bar while fetching, then loads once complete. The fetch does not hold the +editor: releasing it mid-fetch frees it immediately, and the fetch finishes in the background, warming the +cache for the next editor. ### Best Practice: Prepare Early diff --git a/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift b/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift index e27d66f8a..d5c3c6dbc 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift @@ -94,6 +94,20 @@ public actor EditorHTTPClient: EditorHTTPClientProtocol { private let delegate: EditorHTTPClientDelegate? private let requestTimeout: TimeInterval? + /// Requests in flight that an identical `perform(_:)` joins instead of sending again. Every + /// editor and service builds its own client, so this is shared across all of them. + static let inFlightRequests = InFlightTasks() + + /// A request other callers can share: the request as it goes out, and the session it goes + /// out on. `URLRequest`'s own `==` ignores the timeout and the network service type, so + /// those are compared here; it ignores the body too, but a request with one isn't shared. + struct SharedRequest: Hashable, Sendable { + let request: URLRequest + let timeout: TimeInterval + let networkServiceType: URLRequest.NetworkServiceType + let session: ObjectIdentifier + } + public init( urlSession: URLSessionProtocol, authHeader: String, @@ -106,9 +120,63 @@ public actor EditorHTTPClient: EditorHTTPClientProtocol { self.requestTimeout = requestTimeout } + /// Sends `urlRequest`, throwing for a non-2xx status. + /// + /// A request identical to one already in flight joins it rather than going out again, so + /// callers after the same site data — an editor and a prefetch, say — pay for one round + /// trip. Only a safe request without a body is shared, and only between clients no delegate + /// is watching. A request whose cache policy asks to skip the cache goes out alone: its + /// caller wants an answer no older than the call, and a request already in flight may + /// predate a write made since. Cancelling a caller ends its own wait; the request is + /// cancelled once no caller is left waiting on it. public func perform(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { - let configuredRequest = self.configureRequest(urlRequest) + guard let sharedRequest = sharedRequest(forConfigured: configuredRequest) else { + return try await send(configuredRequest) + } + return try await Self.inFlightRequests.value(for: sharedRequest) { _ in + try await self.send(configuredRequest) + } + } + + /// For tests: what `perform(_:)` shares `urlRequest` under, or `nil` if it goes out alone. + func sharedRequest(for urlRequest: URLRequest) -> SharedRequest? { + sharedRequest(forConfigured: configureRequest(urlRequest)) + } + + /// `nil` for a request that must go out alone: an unsafe method or a body, a cache policy + /// that asks for a fresh answer, a delegate that expects to see each request it asked for, + /// or a session that isn't an object — a shared request is keyed by the session's identity, + /// which only an object keeps. + private func sharedRequest(forConfigured request: URLRequest) -> SharedRequest? { + guard delegate == nil, + Self.sharableMethods.contains(request.httpMethod ?? "GET"), + request.httpBody == nil, + request.httpBodyStream == nil, + !Self.freshAnswerPolicies.contains(request.cachePolicy), + type(of: urlSession) is AnyClass + else { + return nil + } + return SharedRequest( + request: request, + timeout: request.timeoutInterval, + networkServiceType: request.networkServiceType, + session: ObjectIdentifier(urlSession as AnyObject) + ) + } + + private static let sharableMethods: Set = ["GET", "HEAD", "OPTIONS"] + + /// The cache policies that ask the server afresh rather than trust a stored response, and + /// so won't take one already on its way. + private static let freshAnswerPolicies: Set = [ + .reloadIgnoringLocalCacheData, + .reloadIgnoringLocalAndRemoteCacheData, + .reloadRevalidatingCacheData, + ] + + private func send(_ configuredRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { let (data, response) = try await self.urlSession.data(for: configuredRequest) self.delegate?.didPerformRequest(configuredRequest, response: response, data: .bytes(data)) diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 5026db0db..548c2d853 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -28,14 +28,14 @@ import UIKit // │ WARMUP MODE │ │ DEPENDENCIES │ │ NO DEPENDENCIES │ // │ (isWarmupMode) │ │ PROVIDED │ │ (Async Flow) │ // │ │ │ (Fast Path) │ │ │ -// │ Load HTML without │ │ │ │ Spawn Task to fetch │ +// │ Load HTML without │ │ │ │ Start a loader to fetch │ // │ any dependencies │ │ loadEditor() │ │ dependencies │ // │ for prewarming │ │ immediately │ │ │ // └────────────────────┘ └────────────────────┘ └───────────────────────────────┘ // │ ▼ // │ ┌───────────────────────────────┐ -// │ │ prepareEditor() │ -// │ │ • Load editor dependencies │ +// │ │ EditorDependencyLoader │ +// │ │ • Fetch editor dependencies │ // │ └───────────────────────────────┘ // │ ▼ // │ ┌───────────────────────────────┐ @@ -66,12 +66,16 @@ import UIKit // // ## Flow 2: No Dependencies (Async Flow) // -// When no dependencies are provided, the controller fetches them asynchronously. +// When no dependencies are provided, an `EditorDependencyLoader` fetches them +// asynchronously and hands them to the fast path. The loader holds the controller +// only weakly, so a controller released mid-fetch is freed at once, not when the +// fetch ends. +// // This is a fallback behaviour – the host app should provide the dependencies if it can, // because it'll be a much better user experience. // @MainActor -public final class EditorViewController: UIViewController, GutenbergEditorControllerDelegate, UIAdaptivePresentationControllerDelegate, UIPopoverPresentationControllerDelegate, UISheetPresentationControllerDelegate { +public final class EditorViewController: UIViewController, GutenbergEditorControllerDelegate, EditorDependencyLoaderDelegate, UIAdaptivePresentationControllerDelegate, UIPopoverPresentationControllerDelegate, UISheetPresentationControllerDelegate { public let webView: WKWebView public var configuration: EditorConfiguration @@ -85,6 +89,9 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// The fetched or provided editor dependencies (settings, assets, preload data). private var dependencies: EditorDependencies? + /// Fetches `dependencies` when none were provided at init. + private var dependencyLoader: EditorDependencyLoader? + /// Error encountered while loading dependencies. private var error: Error? { didSet { @@ -367,23 +374,11 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro if let dependencies { // FAST PATH: Dependencies were provided at init() - load immediately. - // Not cancellable: cancelling mid-`startUploadServer()` silently disables - // native uploads for the session (#357). - Task(priority: .userInitiated) { [weak self] in - do { - try await self?.loadEditor(dependencies: dependencies) - } catch { - self?.failToLoad(error) - } - } + startLoadingEditor(dependencies: dependencies) } else { - // ASYNC FLOW: No dependencies - fetch them, then load as above. - // Not cancellable either, for the same reason plus one: nothing restarts - // the fetch, so the editor never recovers from a cancel. Note that - // `viewDidDisappear` fires when the editor is merely covered. See #651. - Task(priority: .userInitiated) { [weak self] in - await self?.prepareEditor() - } + // ASYNC FLOW: No dependencies - fetch them, then take the fast path. + displayProgressView() + dependencyLoader = EditorDependencyLoader(service: editorService, delegate: self) } } @@ -503,28 +498,25 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro uploadServer?.stop() } - /// Fetches all required dependencies and then loads the editor. - /// - /// This method is the entry point for the **Async Flow** (when no dependencies were provided at init). - @MainActor - private func prepareEditor() async { - self.displayProgressView() - defer { self.hideProgressView() } + // MARK: - Async Flow (EditorDependencyLoaderDelegate) - do { - // EditorService.prepare() fetches dependencies concurrently with progress reporting - let dependencies = try await self.editorService.prepare { @MainActor [weak self] progress in - self?.progressView.setProgress(progress, animated: true) - } + func dependencyLoader(_ loader: EditorDependencyLoader, didUpdate progress: EditorProgress) { + progressView.setProgress(progress, animated: true) + } - // Store dependencies for later use (e.g., HTMLPreviewManager) - self.dependencies = dependencies + func dependencyLoader(_ loader: EditorDependencyLoader, didLoad dependencies: EditorDependencies) { + hideProgressView() - // Continue to the shared loading path - try await self.loadEditor(dependencies: dependencies) - } catch { - self.failToLoad(error) - } + // Store dependencies for later use (e.g., HTMLPreviewManager) + self.dependencies = dependencies + + // Continue to the shared loading path + startLoadingEditor(dependencies: dependencies) + } + + func dependencyLoader(_ loader: EditorDependencyLoader, didFailWith error: any Error) { + hideProgressView() + failToLoad(error) } private func failToLoad(_ error: Error) { @@ -534,6 +526,22 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // MARK: - Shared Loading Path: Load Editor into WebView + /// Runs `loadEditor(dependencies:)` — the step both flows end on. + /// + /// Not cancellable, and it holds the editor until the load returns: cancelling it + /// mid-`startUploadServer()` silently disables native uploads for the session + /// (#357). The hold is short — the server bind is capped by + /// `HTTPServer.defaultStartTimeout`. + private func startLoadingEditor(dependencies: EditorDependencies) { + Task(priority: .userInitiated) { [weak self] in + do { + try await self?.loadEditor(dependencies: dependencies) + } catch { + self?.failToLoad(error) + } + } + } + /// Loads the editor HTML into the WebView with the given dependencies. /// /// This is the **shared loading path** used by both flows after dependencies are available. @@ -675,9 +683,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// Deletes all cached editor data for all sites public static func deleteAllData() throws { - if FileManager.default.directoryExists(at: Paths.defaultCacheRoot) { - try FileManager.default.removeItem(at: Paths.defaultCacheRoot) - } + try EditorURLCache.deleteAll() if FileManager.default.directoryExists(at: Paths.defaultStorageRoot) { try FileManager.default.removeItem(at: Paths.defaultStorageRoot) diff --git a/ios/Sources/GutenbergKit/Sources/Helpers/InFlightTasks.swift b/ios/Sources/GutenbergKit/Sources/Helpers/InFlightTasks.swift new file mode 100644 index 000000000..4d4e13697 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Helpers/InFlightTasks.swift @@ -0,0 +1,166 @@ +import Foundation + +/// Work in flight, one task per key, each shared by every caller asking for that key. +/// +/// A second caller for a key joins the task already running for it rather than starting the +/// same work again. The task belongs to no caller: it runs on its own, so one caller's +/// cancellation can't end it for the others. Cancelling a caller ends that caller's wait at once, +/// and the task is cancelled only when no caller is left waiting on it. A caller that has left +/// hears no further progress, though a progress call already under way when it leaves runs on. +/// A caller that joins at a higher priority than the task's raises the task to match. +final class InFlightTasks: @unchecked Sendable { + + /// Guards `joinable`, and every ``Entry`` and ``Waiter``. + private let lock = NSLock() + + /// The tasks a new caller joins. A task leaves when it finishes, or when its last waiter leaves. + private var joinable: [Key: Entry] = [:] + + /// Returns the value for `key` from the task in flight for it, or from a new one that `run` + /// performs. `progress` hears the task's progress from when this caller joins until it leaves. + func value( + for key: Key, + progress: EditorProgressCallback? = nil, + run: @escaping @Sendable (_ report: @escaping EditorProgressCallback) async throws -> Value + ) async throws -> Value { + let waiter = Waiter(progress: progress) + return try await withTaskCancellationHandler { + try await withCheckedThrowingContinuation { continuation in + join(key, waiter, continuation, run) + } + } onCancel: { + leave(waiter) + } + } + + /// For tests: how many callers are waiting on the task in flight for `key`. + func waiterCount(for key: Key) -> Int { + lock.withLock { joinable[key]?.waiters.count ?? 0 } + } + + /// For tests: the task in flight for `key`. It outlives the wait of a caller that leaves, + /// so a test of what an abandoned task does has to wait for the task itself. + func task(for key: Key) -> Task? { + lock.withLock { joinable[key]?.task } + } + + private func join( + _ key: Key, + _ waiter: Waiter, + _ continuation: CheckedContinuation, + _ run: @escaping @Sendable (_ report: @escaping EditorProgressCallback) async throws -> Value + ) { + let (isCancelled, joined) = lock.withLock { () -> (Bool, Task?) in + // Cancelled before getting here, its `onCancel` has run and found nothing to leave. + guard !Task.isCancelled else { return (true, nil) } + let running = joinable[key] + let entry = running ?? start(key, run) + waiter.continuation = continuation + waiter.entry = entry + entry.waiters.append(waiter) + return (false, running?.task) + } + if isCancelled { + continuation.resume(throwing: CancellationError()) + } else if let joined { + raise(joined) + } + } + + /// Raises `task` to the calling task's priority. A task runs at the priority of the caller + /// that started it, and a caller waiting on a continuation doesn't escalate it the way one + /// awaiting `task.value` would — so an editor joining a background prefetch would otherwise + /// wait at the prefetch's priority. Escalation needs iOS 26 or macOS 26; before that, the + /// task keeps the priority it started with. + private func raise(_ task: Task) { + if #available(iOS 26, macOS 26, *) { + task.escalatePriority(to: Task.currentPriority) + } + } + + /// Starts the task for `key`. Called with `lock` held. + private func start( + _ key: Key, + _ run: @escaping @Sendable (_ report: @escaping EditorProgressCallback) async throws -> Value + ) -> Entry { + let entry = Entry(key: key) + joinable[key] = entry + entry.task = Task { + let result: Result + do { + result = .success(try await run { progress in await self.report(progress, from: entry) }) + } catch { + result = .failure(error) + } + self.finish(entry, with: result) + } + return entry + } + + /// Tells every waiter still waiting, one at a time. Each is checked again just before its + /// turn: an earlier waiter's callback can suspend for as long as it likes, and a later waiter + /// can leave meanwhile — after which its own caller has moved on. + private func report(_ progress: EditorProgress, from entry: Entry) async { + let waiters = lock.withLock { entry.waiters } + for waiter in waiters { + let callback = lock.withLock { entry.waiters.contains { $0 === waiter } ? waiter.progress : nil } + await callback?(progress) + } + } + + private func finish(_ entry: Entry, with result: Result) { + let continuations = lock.withLock { + if joinable[entry.key] === entry { + joinable[entry.key] = nil + } + entry.task = nil + defer { entry.waiters = [] } + return entry.waiters.compactMap(\.continuation) + } + for continuation in continuations { + continuation.resume(with: result) + } + } + + private func leave(_ waiter: Waiter) { + let (continuation, abandoned) = lock.withLock { () -> (CheckedContinuation?, Task?) in + guard let entry = waiter.entry, let index = entry.waiters.firstIndex(where: { $0 === waiter }) else { + return (nil, nil) + } + entry.waiters.remove(at: index) + guard entry.waiters.isEmpty else { + return (waiter.continuation, nil) + } + // No one is left waiting: stop the task, and let the next caller start afresh rather + // than join one on its way out. + if joinable[entry.key] === entry { + joinable[entry.key] = nil + } + return (waiter.continuation, entry.task) + } + continuation?.resume(throwing: CancellationError()) + abandoned?.cancel() + } + + /// One task and the callers waiting on it. Guarded by `lock`. + private final class Entry: @unchecked Sendable { + let key: Key + var task: Task? + var waiters: [Waiter] = [] + + init(key: Key) { + self.key = key + } + } + + /// One ``value(for:progress:run:)`` call. Guarded by `lock`. + private final class Waiter: @unchecked Sendable { + let progress: EditorProgressCallback? + var continuation: CheckedContinuation? + var entry: Entry? + + init(progress: EditorProgressCallback?) { + self.progress = progress + } + } +} diff --git a/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift b/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift index db4927c61..43ae68b5d 100644 --- a/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift +++ b/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift @@ -66,7 +66,10 @@ public struct RESTAPIRepository: Sendable { // MARK: Post @discardableResult public func fetchPost(id: Int) async throws -> EditorURLResponse { - let request = URLRequest(method: .GET, url: self.buildPostUrl(id: id)) + var request = URLRequest(method: .GET, url: self.buildPostUrl(id: id)) + // The post is never cached, and for the same reason never joins a request in flight: + // one started by an editor since closed can predate an edit made in between. + request.cachePolicy = .reloadIgnoringLocalCacheData let response = try await self.httpClient.perform(request) return EditorURLResponse(response) } diff --git a/ios/Sources/GutenbergKit/Sources/Services/EditorDependencyLoader.swift b/ios/Sources/GutenbergKit/Sources/Services/EditorDependencyLoader.swift new file mode 100644 index 000000000..b73e4483e --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Services/EditorDependencyLoader.swift @@ -0,0 +1,50 @@ +import Foundation + +/// Fetches an editor's dependencies without holding the editor. +/// +/// The editor owns its loader, never the reverse: the loader reaches back only through +/// ``delegate``, which is weak and whose requirements are all synchronous. So a released +/// editor is freed at once rather than when the fetch ends — it never awaits the fetch, +/// and nothing the loader calls on it can suspend. Keep it that way by reading +/// `delegate` where it is used: a copy held across the `await` would retain the editor +/// for the whole fetch. +/// +/// The fetch starts on init and is never cancelled, so it keeps the loader alive until it +/// finishes. That is harmless — a loader owns no view, web view, or listener — and a +/// fetch that outlives its editor still warms the cache for the next one. +@MainActor +final class EditorDependencyLoader { + weak let delegate: (any EditorDependencyLoaderDelegate)? + + init(service: EditorService, delegate: any EditorDependencyLoaderDelegate) { + self.delegate = delegate + fetch(from: service) + } + + /// Kept out of `init`, where the strong `delegate` parameter would shadow the weak + /// property — so here the task can reach the delegate only through ``delegate``. + private func fetch(from service: EditorService) { + Task(priority: .userInitiated) { + do { + let dependencies = try await service.prepare { @MainActor progress in + self.delegate?.dependencyLoader(self, didUpdate: progress) + } + delegate?.dependencyLoader(self, didLoad: dependencies) + } catch { + delegate?.dependencyLoader(self, didFailWith: error) + } + } + } +} + +/// Receives an ``EditorDependencyLoader``'s results on the main actor. +/// +/// Every requirement is synchronous, so no call can suspend while holding the delegate. +/// An `async` requirement could, and would keep the editor alive until the call resumed — +/// for the whole fetch, if the call awaited it. +@MainActor +protocol EditorDependencyLoaderDelegate: AnyObject { + func dependencyLoader(_ loader: EditorDependencyLoader, didUpdate progress: EditorProgress) + func dependencyLoader(_ loader: EditorDependencyLoader, didLoad dependencies: EditorDependencies) + func dependencyLoader(_ loader: EditorDependencyLoader, didFailWith error: any Error) +} diff --git a/ios/Sources/GutenbergKit/Sources/Services/EditorService.swift b/ios/Sources/GutenbergKit/Sources/Services/EditorService.swift index d870c197a..0aee8e0fa 100644 --- a/ios/Sources/GutenbergKit/Sources/Services/EditorService.swift +++ b/ios/Sources/GutenbergKit/Sources/Services/EditorService.swift @@ -180,13 +180,14 @@ public actor EditorService { } private func incrementProgress(for weight: DependencyWeights, fraction: Double = 1.0) async { - precondition( - self.progress != nil, - "Progress has not been initialized. This is a bug in the EditorService. Please file an issue." - ) + // Progress can arrive after the `prepare()` it belongs to has returned and cleared it. A + // bundle build shared with another service may already be calling in when this service + // gives up on it, and an overlapping `prepare()` on this service is cleared by whichever + // finishes first. There is nothing left to report to, so drop it. + guard let current = self.progress else { return } let progress = EditorProgress( - completed: self.progress!.completed + Int(weight.rawValue * fraction), - total: self.progress!.total) + completed: current.completed + Int(weight.rawValue * fraction), + total: current.total) self.progress = progress await self.progressCallback?(progress) } diff --git a/ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift b/ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift index 0c720891c..dae3364dd 100644 --- a/ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift +++ b/ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift @@ -9,6 +9,10 @@ public actor EditorAssetLibrary { private let storageRoot: URL private let cachePolicy: EditorCachePolicy + /// Bundle builds in flight, keyed by the directory each writes. Every service builds its own + /// library, so this is shared across all of them. + static let inFlightBuilds = InFlightTasks() + /// Creates a new `EditorAssetLibrary` instance. /// /// - Parameters: @@ -118,6 +122,18 @@ public actor EditorAssetLibrary { return .empty } + // Every build of one manifest writes the same directory, whichever library runs it: + // join a build in flight rather than race a second one into it. + let destination = self.bundleRoot(for: manifest.checksum).standardizedFileURL + return try await Self.inFlightBuilds.value(for: destination, progress: progress) { report in + try await self.build(manifest, reportingTo: report) + } + } + + private func build( + _ manifest: LocalEditorAssetManifest, + reportingTo progress: EditorProgressCallback + ) async throws -> EditorAssetBundle { var complete = 0 let tempDirectory = URL.temporaryDirectory.appending(path: UUID().uuidString) @@ -147,10 +163,16 @@ public actor EditorAssetLibrary { for await _ in group { complete += 1 - await progress?(EditorProgress(completed: complete, total: links.count)) + await progress(EditorProgress(completed: complete, total: links.count)) } } + // The group swallows every per-asset failure, cancellation included, so a + // cancelled build still arrives here with assets missing. Nothing downstream + // checks for them — `readAssetBundles()` reads only the manifest — so publishing + // it would serve the gap on every later launch. + try Task.checkCancellation() + return try bundle.copy(to: self.bundleRoot(for: bundle)) } diff --git a/ios/Sources/GutenbergKit/Sources/Stores/EditorURLCache.swift b/ios/Sources/GutenbergKit/Sources/Stores/EditorURLCache.swift index 40cfad115..fb2545758 100644 --- a/ios/Sources/GutenbergKit/Sources/Stores/EditorURLCache.swift +++ b/ios/Sources/GutenbergKit/Sources/Stores/EditorURLCache.swift @@ -8,11 +8,12 @@ import OSLog /// /// Backed by `SQLiteKVCache`. The cache directory is built as /// `//`, so two caches with different `siteId`s are -/// guaranteed-distinct backing files. The "one instance per backing file" -/// contract from `SQLiteKVCache` still applies for the same `(siteId, -/// parentDirectory)` pair, but the typical call pattern (one cache per -/// `EditorService`, one service per editor view) keeps that contract by -/// construction. +/// guaranteed-distinct backing files. Caches for the same `(siteId, +/// parentDirectory)` pair share one store through +/// `SQLiteKVCache.shared(handle:directory:diskCapacity:)`, which is what keeps +/// that store's "one instance per backing file" contract: every +/// `EditorService` builds its own cache, and a prefetch and an editor for the +/// same site routinely run at once. public struct EditorURLCache: Sendable { /// About enough for 10 sites of cached responses. private static let diskCapacity = Measurement(value: 100, unit: .mebibytes) @@ -38,7 +39,9 @@ public struct EditorURLCache: Sendable { parentDirectory: URL = Paths.defaultCacheRoot, cachePolicy: EditorCachePolicy = .always ) { - self.store = SQLiteKVCache( + // Shared: every service for a site builds its own cache, and two stores on one + // file break each other. + self.store = SQLiteKVCache.shared( handle: "editorurlcache", directory: parentDirectory.appending(path: siteId), diskCapacity: Self.diskCapacity @@ -159,6 +162,17 @@ public struct EditorURLCache: Sendable { try self.store.clear() } + /// Deletes every site's cache under `parentDirectory`, including any still in use. + /// + /// A cache still open when this is called fails every read and write from then on, so its + /// store is no longer shared: a cache created afterwards opens a new file. + static func deleteAll(in parentDirectory: URL = Paths.defaultCacheRoot) throws { + guard FileManager.default.directoryExists(at: parentDirectory) else { return } + // Whether or not the removal finishes: one that fails partway has still deleted files. + defer { SQLiteKVCache.forgetInstances(under: parentDirectory) } + try FileManager.default.removeItem(at: parentDirectory) + } + /// Combines the HTTP method and URL into a single string key. `SQLiteKVCache` /// hashes the key with SHA-256 before binding to SQLite, so length, escaping, /// and encoding aren't concerns here. diff --git a/ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift b/ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift index 706fd9060..9b723a8d3 100644 --- a/ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift +++ b/ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift @@ -42,8 +42,10 @@ import SQLite3 /// instances with different caps would clobber each other's triggers; in the /// best case you get the wrong cap, in the worst case `SQLITE_BUSY` while the /// recreations race. Each backing file must have exactly one owning -/// `SQLiteKVCache` for the lifetime of the process. Not currently enforced at -/// runtime — this is a usage contract. +/// `SQLiteKVCache` at a time. Within a process, ``shared(handle:directory:diskCapacity:)`` +/// enforces that by handing every caller the live instance for its file; `init` +/// doesn't, so use it directly only where nothing else can open the file. Across +/// processes it remains a usage contract. /// /// **Schema migrations.** A `schemaVersion` constant baked into the build is /// compared against `PRAGMA user_version` on open; mismatches drop and recreate @@ -131,6 +133,11 @@ final class SQLiteKVCache: @unchecked Sendable { /// silently wipe users' caches a second time on the next upgrade). private static let schemaVersion: Int32 = 1 + /// How long a statement waits on another connection's lock before failing with + /// `SQLITE_BUSY`. The usual holder is the previous instance on the same file checkpointing + /// its WAL as it closes, which takes milliseconds; this only bounds a pathological one. + private static let busyTimeoutMilliseconds: Int32 = 5_000 + private static let logger = Logger(subsystem: "GutenbergKit", category: "sqlite-kv-cache") /// SQLite C API: signals that bound data should be copied. Reinvented here @@ -203,6 +210,9 @@ final class SQLiteKVCache: @unchecked Sendable { try Self.openAndConfigure(directory: self.directory, filename: self.filename, diskCapacity: self.diskCapacity) } dbResult = result + if case .failure = result { + Self.forget(self) + } return try result.get() } @@ -248,6 +258,14 @@ final class SQLiteKVCache: @unchecked Sendable { throw Error.databaseUnavailable } + // Wait out another connection's lock rather than fail on it: `connection()` caches a + // failure for the life of the cache, so a lock held for milliseconds would break it for + // good. `shared(handle:directory:diskCapacity:)` makes that routine — it hands out a + // fresh instance as soon as the last one is released, while that one's `deinit` may + // still be checkpointing the WAL. Reopening in that window failed 200 times in 200 + // without a timeout, and never with one. + sqlite3_busy_timeout(connection, Self.busyTimeoutMilliseconds) + // Pragmas + schema setup. Pragmas first because `journal_mode` // changes must run with no active transaction. `journal_mode = WAL` // switches from the default rollback-journal to a write-ahead log: @@ -675,6 +693,77 @@ extension SQLiteKVCache.Error: CustomStringConvertible, LocalizedError { extension SQLiteKVCache { + /// The live cache for `handle` in `directory`, created if there is none. + /// + /// Two instances on one file race their opens, and the loser caches its failure and + /// throws for the rest of its life. Measured with two `EditorURLCache`s making their + /// first read at the same moment: at least one ended up broken in 50 runs out of 50. + /// The busy timeout doesn't save it — the switch to WAL fails with `SQLITE_BUSY` + /// without waiting on it. Every caller that can share a file must come through here. + /// + /// Callers sharing a file must agree on `diskCapacity`: one that finds the file open + /// gets the live instance as it is, cap included. + static func shared( + handle: StaticString, + directory: URL = URL.cachesDirectory, + diskCapacity: Measurement + ) -> SQLiteKVCache { + let file = registryKey(directory: directory, filename: "\(handle)".lowercased()) + let bytes = Int(diskCapacity.converted(to: .bytes).value) + return liveInstancesLock.withLock { + if let live = liveInstances[file]?.instance { + assert( + live.diskCapacity == bytes, + "'\(handle)' is already open with a different disk capacity; callers sharing a file must agree on it" + ) + return live + } + let instance = SQLiteKVCache(handle: handle, directory: directory, diskCapacity: bytes) + liveInstances[file] = WeakInstance(instance: instance) + return instance + } + } + + /// Stops `shared` handing out the instances for files under `directory`, so the next caller + /// for each opens it afresh. For a caller that has just deleted `directory`: an instance + /// still open on a deleted file fails every read and write with `SQLITE_IOERR`, and would + /// go on being shared for as long as anything held it. + static func forgetInstances(under directory: URL) { + let root = directory.standardizedFileURL.path(percentEncoded: false) + let prefix = root.hasSuffix("/") ? root : root + "/" + liveInstancesLock.withLock { + liveInstances = liveInstances.filter { !$0.key.hasPrefix(prefix) } + } + } + + /// Stops `shared` handing out `instance`, whose open has failed. It keeps that failure for + /// life, so sharing it would fail every later caller for as long as anything held it. It + /// has closed its handle, so the fresh instance the next caller gets has the file to itself. + /// + /// Called with the instance's `openLock` held, which is why `shared` asks nothing of an + /// instance: an open can take as long as the busy timeout, and `shared` runs on the main + /// thread whenever an editor builds its service. + private static func forget(_ instance: SQLiteKVCache) { + let file = registryKey(directory: instance.directory, filename: instance.filename) + liveInstancesLock.withLock { + if liveInstances[file]?.instance === instance { + liveInstances[file] = nil + } + } + } + + private static func registryKey(directory: URL, filename: String) -> String { + directory.standardizedFileURL.appending(component: filename).path(percentEncoded: false) + } + + /// Weak, so a file no one is using is closed, and opened afresh by the next caller. + nonisolated(unsafe) private static var liveInstances: [String: WeakInstance] = [:] + private static let liveInstancesLock = NSLock() + + private struct WeakInstance { + weak var instance: SQLiteKVCache? + } + /// Convenience initializer that accepts the cap as a /// `Measurement` so callers can write /// `Measurement(value: 100, unit: .mebibytes)` instead of an opaque diff --git a/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift b/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift index 0d49d9bfe..2983c13bd 100644 --- a/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift +++ b/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift @@ -460,6 +460,84 @@ struct EditorHTTPClientTests { #expect(userAgent.contains("macOS/")) #endif } + + // MARK: - Sharing Tests + + @Test("identical requests from clients on one session share a key") + func identicalRequestsShareAKey() async throws { + let session = SpyURLSession() + let request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/types")!) + let first = await EditorHTTPClient(urlSession: session, authHeader: "Bearer a").sharedRequest(for: request) + let second = await EditorHTTPClient(urlSession: session, authHeader: "Bearer a").sharedRequest(for: request) + #expect(first != nil) + #expect(first == second) + } + + @Test("requests with other credentials, sessions, or timeouts don't share") + func requestsWithOtherCredentialsSessionsOrTimeoutsDontShare() async throws { + let session = SpyURLSession() + let request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/types")!) + let key = await EditorHTTPClient(urlSession: session, authHeader: "Bearer a").sharedRequest(for: request) + + #expect(await EditorHTTPClient(urlSession: session, authHeader: "Bearer b").sharedRequest(for: request) != key) + #expect(await EditorHTTPClient(urlSession: SpyURLSession(), authHeader: "Bearer a").sharedRequest(for: request) != key) + #expect(await EditorHTTPClient(urlSession: session, authHeader: "Bearer a", requestTimeout: 5).sharedRequest(for: request) != key) + } + + @Test("only safe requests without a body, from a client no delegate watches, are shared") + func onlySafeUnwatchedRequestsAreShared() async throws { + let session = SpyURLSession() + let client = EditorHTTPClient(urlSession: session, authHeader: "Bearer a") + let url = URL(string: "https://example.com/wp-json/wp/v2/settings")! + + #expect(await client.sharedRequest(for: URLRequest(method: .OPTIONS, url: url)) != nil) + #expect(await client.sharedRequest(for: URLRequest(method: .POST, url: url)) == nil) + + var withBody = URLRequest(url: url) + withBody.httpBody = Data("{}".utf8) + #expect(await client.sharedRequest(for: withBody) == nil) + + let watched = EditorHTTPClient(urlSession: session, authHeader: "Bearer a", delegate: SpyHTTPClientDelegate()) + #expect(await watched.sharedRequest(for: URLRequest(url: url)) == nil) + } + + @Test("a request that asks to skip the cache goes out alone") + func aRequestThatSkipsTheCacheGoesOutAlone() async throws { + let client = EditorHTTPClient(urlSession: SpyURLSession(), authHeader: "Bearer a") + var request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/posts/5")!) + + let freshAnswerPolicies: [URLRequest.CachePolicy] = [ + .reloadIgnoringLocalCacheData, .reloadIgnoringLocalAndRemoteCacheData, .reloadRevalidatingCacheData, + ] + for policy in freshAnswerPolicies { + request.cachePolicy = policy + #expect(await client.sharedRequest(for: request) == nil) + } + + request.cachePolicy = .returnCacheDataElseLoad + #expect(await client.sharedRequest(for: request) != nil) + } + + @Test("identical requests in flight go out once") + func identicalRequestsInFlightGoOutOnce() async throws { + let session = ParkedURLSession() + defer { session.release() } + let request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/types")!) + let clients = [ + EditorHTTPClient(urlSession: session, authHeader: "Bearer a"), + EditorHTTPClient(urlSession: session, authHeader: "Bearer a"), + ] + let key = try #require(await clients[0].sharedRequest(for: request)) + + let callers = clients.map { client in Task { try await client.perform(request) } } + try await waitUntil { EditorHTTPClient.inFlightRequests.waiterCount(for: key) == 2 } + + session.release() // fails the parked request, for every caller waiting on it + for caller in callers { + await #expect(throws: URLError.self) { try await caller.value } + } + #expect(session.requestCount == 1) + } } fileprivate extension EditorResponseData { diff --git a/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift b/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift index a672b38d1..4807715a6 100644 --- a/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift +++ b/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift @@ -35,33 +35,33 @@ struct EditorViewControllerLifecycleTests: MakesTestFixtures { #expect(!cancelled) } - /// Why `deinit` can't cancel the fetch: `await self?.prepareEditor()` keeps the - /// editor alive until the load finishes, so `deinit` only runs once it's over. + /// The loader owns the fetch and reaches the editor only weakly, so a released + /// editor is freed while its fetch is still parked — and the fetch keeps running. @MainActor - @Test("the in-flight fetch keeps the editor alive until it finishes") - func theInFlightFetchKeepsTheEditorAlive() async throws { + @Test("releasing the editor mid-fetch frees it, and leaves the fetch running") + func releasingTheEditorMidFetchFreesIt() async throws { let session = ParkedURLSession() let configuration = makeIsolatedConfiguration() defer { removeStorage(for: configuration) } - // Safety net if a throw skips the `release()` below; calling it twice is fine. defer { session.release() } var editor: EditorViewController? = makeEditor(configuration: configuration, session: session) weak let releasedEditor = editor - _ = editor?.view + _ = editor?.view // triggers `viewDidLoad`, which starts the fetch try await session.waitUntilStarted() + // Polled rather than checked once, so it doesn't depend on exactly when UIKit + // lets go. The fetch stays parked throughout, so it can't be what lets go. editor = nil - try await Task.sleep(for: .milliseconds(250)) - #expect(releasedEditor != nil, "the fetch should hold the editor alive") - - session.release() let clock = ContinuousClock() - let deadline = clock.now + .seconds(10) + let deadline = clock.now + .seconds(2) while releasedEditor != nil && clock.now < deadline { try await Task.sleep(for: .milliseconds(20)) } - #expect(releasedEditor == nil, "the editor should be freed once the fetch ends") + #expect(releasedEditor == nil, "the fetch should not hold the editor") + + let cancelled = await session.waitUntilCancelled(timeout: .milliseconds(250)) + #expect(!cancelled, "freeing the editor should not cancel the fetch") } /// A unique `siteId` per call, so no earlier run's cache can serve the fetch. @@ -92,70 +92,4 @@ struct EditorViewControllerLifecycleTests: MakesTestFixtures { } } -/// A `URLSessionProtocol` whose requests hang until `release()`, so a fetch stays in -/// flight for as long as the test needs. Records whether any request was cancelled. -private final class ParkedURLSession: URLSessionProtocol, @unchecked Sendable { - private let lock = NSLock() - private var started = false - private var cancelled = false - private var released = false - - private var isStarted: Bool { lock.withLock { started } } - private var isCancelled: Bool { lock.withLock { cancelled } } - private var isReleased: Bool { lock.withLock { released } } - - func data(for request: URLRequest) async throws -> (Data, URLResponse) { - try await park() - } - - func download(for request: URLRequest, delegate: (any URLSessionTaskDelegate)?) async throws -> (URL, URLResponse) { - try await park() - } - - /// Makes every parked request fail, so the fetch ends. Always call it: a request - /// left parked keeps its editor alive for the rest of the run. - func release() { - lock.withLock { released = true } - } - - /// Suspends until `release()` or until the calling task is cancelled. - /// `Never` because every exit throws, so it fits both methods' return types. - private func park() async throws -> Never { - lock.withLock { started = true } - while !isReleased { - do { - try await Task.sleep(for: .milliseconds(20)) - } catch { - lock.withLock { cancelled = true } - throw URLError(.cancelled) - } - } - throw URLError(.networkConnectionLost) - } - - func waitUntilStarted(timeout: Duration = .seconds(10)) async throws { - let clock = ContinuousClock() - let deadline = clock.now + timeout - while clock.now < deadline { - if isStarted { return } - try await Task.sleep(for: .milliseconds(20)) - } - throw ParkedURLSessionTimeout.requestNeverStarted - } - - func waitUntilCancelled(timeout: Duration) async -> Bool { - let clock = ContinuousClock() - let deadline = clock.now + timeout - while clock.now < deadline { - if isCancelled { return true } - try? await Task.sleep(for: .milliseconds(20)) - } - return isCancelled - } -} - -private enum ParkedURLSessionTimeout: Error { - case requestNeverStarted -} - #endif diff --git a/ios/Tests/GutenbergKitTests/Helpers/ParkedURLSession.swift b/ios/Tests/GutenbergKitTests/Helpers/ParkedURLSession.swift new file mode 100644 index 000000000..be31af8be --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Helpers/ParkedURLSession.swift @@ -0,0 +1,77 @@ +import Foundation +@testable import GutenbergKit + +/// A `URLSessionProtocol` whose requests never finish until the test lets them, so +/// work started against it stays in flight for as long as the test needs, and which +/// records whether the task waiting on a request was cancelled. +final class ParkedURLSession: URLSessionProtocol, @unchecked Sendable { + private let lock = NSLock() + private var started = false + private var cancelled = false + private var released = false + private var requests = 0 + + private var isStarted: Bool { lock.withLock { started } } + private var isCancelled: Bool { lock.withLock { cancelled } } + private var isReleased: Bool { lock.withLock { released } } + + /// How many requests have been made against this session. + var requestCount: Int { lock.withLock { requests } } + + func data(for request: URLRequest) async throws -> (Data, URLResponse) { + try await park() + } + + func download(for request: URLRequest, delegate: (any URLSessionTaskDelegate)?) async throws -> (URL, URLResponse) { + try await park() + } + + /// Lets every parked request fail, so the work waiting on them — and the task + /// running it — finishes. Always call this: a request left parked stays in + /// flight for the rest of the run. + func release() { + lock.withLock { released = true } + } + + /// Suspends until `release()` or until the calling task is cancelled. + /// `Never` because every exit throws — it satisfies both return types. + private func park() async throws -> Never { + lock.withLock { + started = true + requests += 1 + } + while !isReleased { + do { + try await Task.sleep(for: .milliseconds(20)) + } catch { + lock.withLock { cancelled = true } + throw URLError(.cancelled) + } + } + throw URLError(.networkConnectionLost) + } + + func waitUntilStarted(timeout: Duration = .seconds(10)) async throws { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isStarted { return } + try await Task.sleep(for: .milliseconds(20)) + } + throw ParkedURLSessionTimeout.requestNeverStarted + } + + func waitUntilCancelled(timeout: Duration) async -> Bool { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isCancelled { return true } + try? await Task.sleep(for: .milliseconds(20)) + } + return isCancelled + } +} + +enum ParkedURLSessionTimeout: Error { + case requestNeverStarted +} diff --git a/ios/Tests/GutenbergKitTests/InFlightTasksTests.swift b/ios/Tests/GutenbergKitTests/InFlightTasksTests.swift new file mode 100644 index 000000000..72376d2b2 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/InFlightTasksTests.swift @@ -0,0 +1,228 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +@Suite("InFlightTasks", .timeLimit(.minutes(1))) +struct InFlightTasksTests { + + /// Each test's own table, so tests running in parallel can't meet in it. + private let tasks = InFlightTasks() + + // MARK: - Sharing + + @Test("callers for the same key share one task, and all hear its progress") + func callersForTheSameKeyShareOneTask() async throws { + let work = ParkedWork() + let (first, second) = (ProgressTracker(), ProgressTracker()) + let a = startCaller(for: "key", running: work, progress: first) + let b = startCaller(for: "key", running: work, progress: second) + try await waitUntil { tasks.waiterCount(for: "key") == 2 } + try await waitUntil { !first.updates.isEmpty && !second.updates.isEmpty } + + work.release() + #expect(try await a.value == ParkedWork.value) + #expect(try await b.value == ParkedWork.value) + #expect(work.runs == 1) + } + + @Test("callers for different keys run separately") + func callersForDifferentKeysRunSeparately() async throws { + let work = ParkedWork() + let one = startCaller(for: "one", running: work) + let other = startCaller(for: "other", running: work) + try await waitUntil { work.runs == 2 } + + work.release() + #expect(try await one.value == ParkedWork.value) + #expect(try await other.value == ParkedWork.value) + } + + @Test("a failed task fails every caller waiting on it") + func aFailedTaskFailsEveryCaller() async throws { + let work = ParkedWork() + let a = startCaller(for: "key", running: work) + let b = startCaller(for: "key", running: work) + try await waitUntil { tasks.waiterCount(for: "key") == 2 } + + work.release(throwing: URLError(.timedOut)) + await #expect(throws: URLError.self) { try await a.value } + await #expect(throws: URLError.self) { try await b.value } + } + + // MARK: - Cancellation + + @Test("cancelling a caller ends its wait at once, and leaves the task running for the rest") + func cancellingACallerLeavesTheTaskRunning() async throws { + let work = ParkedWork() + let leaving = startCaller(for: "key", running: work) + let staying = startCaller(for: "key", running: work) + try await waitUntil { tasks.waiterCount(for: "key") == 2 } + + leaving.cancel() + // Returns while the task is still parked — it doesn't wait for it. + await #expect(throws: CancellationError.self) { try await leaving.value } + #expect(!work.wasCancelled) + + work.release() + #expect(try await staying.value == ParkedWork.value) + } + + @Test("the last caller leaving cancels the task, and the next caller starts afresh") + func theLastCallerLeavingCancelsTheTask() async throws { + let work = ParkedWork() + let first = startCaller(for: "key", running: work) + try await waitUntil { work.runs == 1 } + + first.cancel() + await #expect(throws: CancellationError.self) { try await first.value } + try await waitUntil { work.wasCancelled } + + let next = startCaller(for: "key", running: work) + try await waitUntil { work.runs == 2 } + work.release() + #expect(try await next.value == ParkedWork.value) + } + + @Test("a caller that has left hears no more progress, even from a report already under way") + func aCallerThatHasLeftHearsNoMoreProgress() async throws { + let work = ParkedWork() + let gate = ProgressGate() + let leaver = ProgressTracker() + let staying = startCaller(for: "key", running: work, onProgress: { await gate.pass($0) }) + try await waitUntil { tasks.waiterCount(for: "key") == 1 } + let leaving = startCaller(for: "key", running: work, progress: leaver) + try await waitUntil { !leaver.updates.isEmpty } + + // Hold a report in the staying caller's callback. The leaving caller had already heard + // one, so it is on this report's list too, waiting its turn. + gate.close() + try await waitUntil { gate.isHolding } + leaving.cancel() + await #expect(throws: CancellationError.self) { try await leaving.value } + let heard = leaver.count + + // Let the held report finish and the next one start, so the leaving caller's turn is past. + let passes = gate.passes + gate.open() + try await waitUntil { gate.passes >= passes + 2 } + #expect(leaver.count == heard, "the leaving caller should hear nothing once it has left") + + work.release() + #expect(try await staying.value == ParkedWork.value) + } + + // MARK: - Priority + + @Test("a caller joining at a higher priority raises the task to match") + func aHigherPriorityCallerRaisesTheTask() async throws { + guard #available(iOS 26, macOS 26, *) else { return } + let work = ParkedWork() + let low = startCaller(for: "key", running: work, priority: .utility) + try await waitUntil { work.runs == 1 } + #expect(work.priority.rawValue < TaskPriority.userInitiated.rawValue) + + let high = startCaller(for: "key", running: work, priority: .userInitiated) + try await waitUntil { work.priority.rawValue >= TaskPriority.userInitiated.rawValue } + + work.release() + #expect(try await low.value == ParkedWork.value) + #expect(try await high.value == ParkedWork.value) + } + + // MARK: - Helpers + + /// A caller waiting on `key`, which runs `work` if nothing is in flight for it yet. It hears + /// progress through `onProgress`, or else records it in `progress`. + private func startCaller( + for key: String, + running work: ParkedWork, + priority: TaskPriority? = nil, + progress: ProgressTracker? = nil, + onProgress: EditorProgressCallback? = nil + ) -> Task { + let callback: EditorProgressCallback? = onProgress ?? progress.map { tracker in + { @Sendable (update: EditorProgress) async in tracker.append(update) } + } + return Task(priority: priority) { [tasks] in + try await tasks.value(for: key, progress: callback) { report in + try await work.run(reporting: report) + } + } + } +} + +/// A progress callback the test can close, holding whichever report reaches it until it reopens. +private final class ProgressGate: @unchecked Sendable { + private let lock = NSLock() + private var closed = false + private var holding = false + private var passCount = 0 + + /// Whether a report is held here right now. + var isHolding: Bool { lock.withLock { holding } } + + /// How many reports have been let through. + var passes: Int { lock.withLock { passCount } } + + func close() { lock.withLock { closed = true } } + func open() { lock.withLock { closed = false } } + + func pass(_ progress: EditorProgress) async { + while lock.withLock({ holding = closed; return closed }) { + try? await Task.sleep(for: .milliseconds(5)) + } + lock.withLock { + holding = false + passCount += 1 + } + } +} + +/// Work that runs until released: it counts its runs, reports progress while it waits, and +/// records whether it was cancelled. +private final class ParkedWork: @unchecked Sendable { + static let value = 42 + + private let lock = NSLock() + private var runCount = 0 + private var cancelled = false + private var released = false + private var failure: (any Error)? + private var latestPriority = TaskPriority.medium + + var runs: Int { lock.withLock { runCount } } + var wasCancelled: Bool { lock.withLock { cancelled } } + + /// The priority the latest run was going at when it last checked. + var priority: TaskPriority { lock.withLock { latestPriority } } + + /// Lets every run finish: with `failure` if there is one, or with ``value``. + func release(throwing failure: (any Error)? = nil) { + lock.withLock { + self.failure = failure + released = true + } + } + + func run(reporting report: EditorProgressCallback) async throws -> Int { + lock.withLock { + latestPriority = Task.currentPriority + runCount += 1 + } + while !lock.withLock({ released }) { + lock.withLock { latestPriority = Task.currentPriority } + await report(EditorProgress(completed: 1, total: 100)) + do { + try await Task.sleep(for: .milliseconds(10)) + } catch { + lock.withLock { cancelled = true } + throw error + } + } + if let failure = lock.withLock({ failure }) { + throw failure + } + return Self.value + } +} diff --git a/ios/Tests/GutenbergKitTests/Services/EditorDependencyLoaderTests.swift b/ios/Tests/GutenbergKitTests/Services/EditorDependencyLoaderTests.swift new file mode 100644 index 000000000..8e699dc0b --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Services/EditorDependencyLoaderTests.swift @@ -0,0 +1,96 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +/// The loader's contract with its owner: it reports back, and it never holds the owner — +/// the property that lets a released editor go while its fetch is still in flight. +@Suite("EditorDependencyLoader") +struct EditorDependencyLoaderTests: MakesTestFixtures { + static let testSiteURL = URL(string: "https://example.com")! + static let testApiRoot = URL(string: "https://example.com/wp-json")! + + @MainActor + @Test("delivers the dependencies it fetched") + func deliversTheDependencies() async throws { + // Offline mode resolves without a request, so the fetch succeeds. + let configuration = makeConfigurationBuilder().setIsOfflineModeEnabled(true).build() + let owner = LoaderOwner(service: makeService(for: configuration)) + + try await owner.waitUntilFinished() + #expect(owner.dependencies != nil) + #expect(owner.error == nil) + } + + @MainActor + @Test("delivers the error when the fetch fails") + func deliversTheError() async throws { + let session = ParkedURLSession() + defer { session.release() } + let owner = LoaderOwner(service: makeService(session: session)) + try await session.waitUntilStarted() + + session.release() // fails every parked request + try await owner.waitUntilFinished() + #expect(owner.error != nil) + #expect(owner.dependencies == nil) + } + + @MainActor + @Test("releasing its owner mid-fetch frees the owner, and leaves the fetch running") + func releasingTheOwnerMidFetchFreesIt() async throws { + let session = ParkedURLSession() + defer { session.release() } + var owner: LoaderOwner? = LoaderOwner(service: makeService(session: session)) + weak let releasedOwner = owner + try await session.waitUntilStarted() + + owner = nil + #expect(releasedOwner == nil, "the fetch should not hold its owner") + + let cancelled = await session.waitUntilCancelled(timeout: .milliseconds(250)) + #expect(!cancelled, "freeing the owner should not cancel the fetch") + } + + /// A service whose every request lands in `session`, with storage no other test shares. + private func makeService(session: ParkedURLSession) -> EditorService { + let configuration = makeConfiguration() + return EditorService( + configuration: configuration, + httpClient: EditorHTTPClient(urlSession: session, authHeader: configuration.authHeader), + storageRoot: .randomTemporaryDirectory, + cacheRoot: .randomTemporaryDirectory + ) + } +} + +/// Stands in for `EditorViewController`: owns its loader, and records what it is told. +@MainActor +private final class LoaderOwner: EditorDependencyLoaderDelegate { + private var loader: EditorDependencyLoader? + private(set) var dependencies: EditorDependencies? + private(set) var error: (any Error)? + + init(service: EditorService) { + loader = EditorDependencyLoader(service: service, delegate: self) + } + + func dependencyLoader(_ loader: EditorDependencyLoader, didUpdate progress: EditorProgress) {} + + func dependencyLoader(_ loader: EditorDependencyLoader, didLoad dependencies: EditorDependencies) { + self.dependencies = dependencies + } + + func dependencyLoader(_ loader: EditorDependencyLoader, didFailWith error: any Error) { + self.error = error + } + + func waitUntilFinished() async throws { + let clock = ContinuousClock() + let deadline = clock.now + .seconds(10) + while dependencies == nil && error == nil && clock.now < deadline { + try await Task.sleep(for: .milliseconds(20)) + } + try #require(dependencies != nil || error != nil, "the loader never reported back") + } +} diff --git a/ios/Tests/GutenbergKitTests/Services/EditorServiceTests.swift b/ios/Tests/GutenbergKitTests/Services/EditorServiceTests.swift index 9635ba5ca..0355f4b15 100644 --- a/ios/Tests/GutenbergKitTests/Services/EditorServiceTests.swift +++ b/ios/Tests/GutenbergKitTests/Services/EditorServiceTests.swift @@ -112,6 +112,32 @@ struct EditorServiceTests: MakesTestFixtures { #expect(!postRequests.isEmpty, "Should request /posts/123 for positive post IDs") } + // MARK: - Progress + + /// The first `prepare()` to finish clears the service's progress while the other still has + /// progress to report, so `incrementProgress` has to drop late progress rather than trap on it. + /// A shared bundle build delivers the same late progress to a service whose `prepare()` has + /// given up on it. + @Test("overlapping prepare() calls on one service don't trap on each other's progress") + func overlappingPrepareCallsDontTrap() async throws { + let client = GatedHTTPClient(respond: Self.editorServiceResponseHandler) + let service = EditorService( + configuration: makeConfiguration(), + httpClient: client, + storageRoot: .randomTemporaryDirectory, + cacheRoot: .randomTemporaryDirectory + ) + + let first = Task { try await GatedHTTPClient.$caller.withValue("first") { try await service.prepare() } } + let second = Task { try await GatedHTTPClient.$caller.withValue("second") { try await service.prepare() } } + try await waitUntil { client.isHolding("first") && client.isHolding("second") } + + client.release("first") + _ = try await first.value + client.release("second") + _ = try await second.value + } + // MARK: - Test Helpers /// URL-based response handler for EditorService.prepare() tests. @@ -138,3 +164,43 @@ struct EditorServiceTests: MakesTestFixtures { } } } + +/// Answers every request from `respond`, but holds each one until the test releases the caller +/// that made it — named by ``caller``, which a test sets around the work it starts. +private final class GatedHTTPClient: EditorHTTPClientProtocol, @unchecked Sendable { + @TaskLocal static var caller = "" + + private let respond: @Sendable (URL) -> Data + private let lock = NSLock() + private var holding: [String: Int] = [:] + private var released: Set = [] + + init(respond: @escaping @Sendable (URL) -> Data) { + self.respond = respond + } + + /// Whether a request from `caller` is being held. + func isHolding(_ caller: String) -> Bool { + lock.withLock { holding[caller, default: 0] > 0 } + } + + /// Lets every request from `caller`, held or still to come, through. + func release(_ caller: String) { + lock.withLock { _ = released.insert(caller) } + } + + func perform(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { + let caller = Self.caller + lock.withLock { holding[caller, default: 0] += 1 } + defer { lock.withLock { holding[caller, default: 0] -= 1 } } + while !lock.withLock({ released.contains(caller) }) { + try await Task.sleep(for: .milliseconds(5)) + } + let url = try #require(urlRequest.url) + return (respond(url), try #require(HTTPURLResponse(url: url, statusCode: 200, httpVersion: nil, headerFields: nil))) + } + + func download(_ urlRequest: URLRequest) async throws -> (URL, HTTPURLResponse) { + throw URLError(.unsupportedURL) + } +} diff --git a/ios/Tests/GutenbergKitTests/Services/RESTAPIRepositoryTests.swift b/ios/Tests/GutenbergKitTests/Services/RESTAPIRepositoryTests.swift index 09efb4ca8..f9c195129 100644 --- a/ios/Tests/GutenbergKitTests/Services/RESTAPIRepositoryTests.swift +++ b/ios/Tests/GutenbergKitTests/Services/RESTAPIRepositoryTests.swift @@ -23,6 +23,29 @@ struct RESTAPIRepositoryTests: MakesTestFixtures { #expect(mockClient.getCallCount == 1) } + /// An editor reopened on a post must not join the request an editor since closed still has + /// in flight for it: that one can predate an edit made in between. + @Test("fetchPost goes out for every caller, even with an identical request in flight") + func fetchPostIsNeverShared() async throws { + let session = ParkedURLSession() + defer { session.release() } + let configuration = makeConfiguration(postID: 5) + let repositories = (0..<2).map { _ in + makeRepository( + configuration: configuration, + httpClient: EditorHTTPClient(urlSession: session, authHeader: configuration.authHeader) + ) + } + + let fetches = repositories.map { repository in Task { try await repository.fetchPost(id: 5) } } + try await waitUntil { session.requestCount == 2 } + + session.release() // fails both parked requests + for fetch in fetches { + await #expect(throws: URLError.self) { try await fetch.value } + } + } + // MARK: - fetchEditorSettings Tests @Test("fetchEditorSettings returns undefined when theme styles disabled") diff --git a/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift b/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift index 320f953bc..2dfa7640d 100644 --- a/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift +++ b/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift @@ -873,6 +873,68 @@ struct EditorAssetLibraryTests { let failedScriptPath = bundleRoot.appending(path: "/stats.js") #expect(!FileManager.default.fileExists(at: failedScriptPath)) } + + @Test("buildBundle publishes nothing when it is cancelled mid-download") + func buildBundlePublishesNothingWhenCancelled() async throws { + let manifestJSON = """ + { + "scripts": "", + "styles": "", + "allowed_block_types": ["core/paragraph"] + } + """ + let manifest = try LocalEditorAssetManifest( + remoteManifest: RemoteEditorAssetManifest(data: Data(manifestJSON.utf8)) + ) + + let session = ParkedURLSession() + defer { session.release() } + let library = makeLibrary(httpClient: EditorHTTPClient(urlSession: session, authHeader: "Bearer test-token")) + let destination = await library.bundleRoot(for: manifest.checksum).standardizedFileURL + + let build = Task { try await library.buildBundle(for: manifest) } + try await session.waitUntilStarted() + let abandoned = try #require(EditorAssetLibrary.inFlightBuilds.task(for: destination)) + build.cancel() + + // The cancelled download is swallowed like any failed asset; the build must + // still refuse to publish, or every later launch serves the gap. + await #expect(throws: CancellationError.self) { try await build.value } + // The caller's wait ends before the build it abandoned is cancelled, so wait for the + // build itself: checked any sooner, a build about to publish hasn't yet. + await abandoned.value + #expect(try await library.readAssetBundles().isEmpty) + } + + @Test("builds of one bundle share one build, whichever library runs them") + func buildsOfOneBundleShareOneBuild() async throws { + let manifestJSON = """ + { + "scripts": "", + "styles": "", + "allowed_block_types": ["core/paragraph"] + } + """ + let manifest = try LocalEditorAssetManifest( + remoteManifest: RemoteEditorAssetManifest(data: Data(manifestJSON.utf8)) + ) + + let session = ParkedURLSession() + defer { session.release() } + let storageRoot = URL.randomTemporaryDirectory + let libraries = [ + makeLibrary(httpClient: EditorHTTPClient(urlSession: session, authHeader: "Bearer test-token"), storageRoot: storageRoot), + makeLibrary(httpClient: EditorHTTPClient(urlSession: session, authHeader: "Bearer test-token"), storageRoot: storageRoot), + ] + let destination = await libraries[0].bundleRoot(for: manifest.checksum).standardizedFileURL + + let builds = libraries.map { library in Task { try await library.buildBundle(for: manifest) } } + try await waitUntil { EditorAssetLibrary.inFlightBuilds.waiterCount(for: destination) == 2 } + + session.release() // fails the parked download, which a build tolerates + #expect(try await builds[0].value == builds[1].value) + #expect(session.requestCount == 1) + } } // MARK: - Progress Tracker for Tests diff --git a/ios/Tests/GutenbergKitTests/Stores/EditorURLCacheTests.swift b/ios/Tests/GutenbergKitTests/Stores/EditorURLCacheTests.swift index dd278483c..c1266c67f 100644 --- a/ios/Tests/GutenbergKitTests/Stores/EditorURLCacheTests.swift +++ b/ios/Tests/GutenbergKitTests/Stores/EditorURLCacheTests.swift @@ -444,4 +444,66 @@ struct EditorURLCacheAlwaysPolicyTests { let tenYearsLater = self.referenceDate.addingTimeInterval(10 * 365 * 24 * 60 * 60) #expect(try cache.hasData(for: testURL, httpMethod: .GET, currentDate: tenYearsLater) == true) } + + // MARK: - One store per site + + /// Every service builds its own cache for a site, so caches share a file — and two stores on + /// one file race their opens, the loser failing for the rest of its life. With a store per + /// cache, at least one broke in every run. + @Test("caches for one site can be opened and used at the same time") + func cachesForOneSiteCanBeUsedAtTheSameTime() async throws { + for _ in 0..<20 { + let parent = URL.randomTemporaryDirectory + let caches = [ + EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always), + EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always), + ] + // Each makes its first read at the same moment, as two services starting a fetch do. + await withTaskGroup { group in + for cache in caches { + group.addTask { _ = try? cache.response(for: testURL, httpMethod: .GET) } + } + } + for cache in caches { + try cache.store(makeResponse(), for: testURL, httpMethod: .GET) + } + } + } + + /// A cache still open on a deleted file fails every read and write. While its store was + /// still shared, so did every cache created after the delete, for as long as it was held. + @Test("a cache created after deleteAll opens a new file") + func aCacheCreatedAfterDeleteAllOpensANewFile() throws { + let parent = URL.randomTemporaryDirectory + let stale = EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always) + try stale.store(makeResponse(), for: testURL, httpMethod: .GET) + + try EditorURLCache.deleteAll(in: parent) + + // Held throughout, as an editor still open or a fetch still running holds its cache. + try withExtendedLifetime(stale) { + let cache = EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always) + #expect(try cache.response(for: testURL, httpMethod: .GET) == nil) + let response = makeResponse() + try cache.store(response, for: testURL, httpMethod: .GET) + #expect(try cache.response(for: testURL, httpMethod: .GET) == response) + } + } + + @Test("caches for one site can write at the same time") + func cachesForOneSiteCanWriteAtTheSameTime() async throws { + let parent = URL.randomTemporaryDirectory + let caches = [ + EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always), + EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always), + ] + try await withThrowingTaskGroup { group in + for index in 0..<200 { + let cache = caches[index % caches.count] + let url = testURL.appending(path: "\(index % 20)") + group.addTask { try cache.store(makeResponse(), for: url, httpMethod: .GET) } + } + try await group.waitForAll() + } + } } diff --git a/ios/Tests/GutenbergKitTests/Stores/SQLiteKVCacheTests.swift b/ios/Tests/GutenbergKitTests/Stores/SQLiteKVCacheTests.swift index ad6757900..95d741438 100644 --- a/ios/Tests/GutenbergKitTests/Stores/SQLiteKVCacheTests.swift +++ b/ios/Tests/GutenbergKitTests/Stores/SQLiteKVCacheTests.swift @@ -651,4 +651,114 @@ struct SQLiteKVCacheTests { let expected = try encoder.encode(meta) #expect(entry.metadata == expected) } + + // MARK: - One instance per file + + @Test("shared hands every caller the live instance for a file") + func sharedHandsOutOneInstancePerFile() { + let directory = URL.randomTemporaryDirectory + let capacity = Measurement(value: 1, unit: .mebibytes) + let store = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + + #expect(SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) === store) + #expect(SQLiteKVCache.shared(handle: "TEST", directory: directory, diskCapacity: capacity) === store) + #expect(SQLiteKVCache.shared(handle: "other", directory: directory, diskCapacity: capacity) !== store) + #expect(SQLiteKVCache.shared(handle: "test", directory: .randomTemporaryDirectory, diskCapacity: capacity) !== store) + } + + @Test("shared opens a file afresh once no one is using it") + func sharedReopensAFileNoOneIsUsing() throws { + let directory = URL.randomTemporaryDirectory + let capacity = Measurement(value: 1, unit: .mebibytes) + var store: SQLiteKVCache? = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + try store?.put(key: "durable", storageDate: referenceDate, metadata: Data(), value: Data("v")) + weak let released = store + + store = nil + #expect(released == nil, "sharing should not keep a file open") + + let reopened = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + #expect(try reopened.get(key: "durable")?.value == Data("v")) + } + + /// An instance keeps a failed open for life. While `shared` went on handing it out, every + /// later caller failed with it for as long as anything held it, though the file could by + /// then be opened. + @Test("shared opens a file afresh after an open that failed") + func sharedReopensAFileAfterAFailedOpen() throws { + let directory = URL.randomTemporaryDirectory + let capacity = Measurement(value: 1, unit: .mebibytes) + // A directory where the database file belongs fails the open. + let obstacle = directory.appending(component: "test.sqlite") + try FileManager.default.createDirectory(at: obstacle, withIntermediateDirectories: true) + let failed = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + #expect(throws: SQLiteKVCache.Error.self) { try failed.get(key: "k") } + + try FileManager.default.removeItem(at: obstacle) + try withExtendedLifetime(failed) { + let reopened = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + #expect(reopened !== failed) + try reopened.put(key: "k", storageDate: referenceDate, metadata: Data(), value: Data("v")) + #expect(try reopened.get(key: "k")?.value == Data("v")) + } + } + + @Test("forgetInstances stops sharing the instances under a directory, and no others") + func forgetInstancesIsScopedToADirectory() { + let root = URL.randomTemporaryDirectory + // Beside `root` rather than under it, though its path starts with `root`'s. + let beside = root.deletingLastPathComponent().appending(path: root.lastPathComponent + "-beside") + let capacity = Measurement(value: 1, unit: .mebibytes) + let under = SQLiteKVCache.shared(handle: "test", directory: root.appending(path: "site"), diskCapacity: capacity) + let other = SQLiteKVCache.shared(handle: "test", directory: beside.appending(path: "site"), diskCapacity: capacity) + + SQLiteKVCache.forgetInstances(under: root) + + #expect(SQLiteKVCache.shared(handle: "test", directory: root.appending(path: "site"), diskCapacity: capacity) !== under) + #expect(SQLiteKVCache.shared(handle: "test", directory: beside.appending(path: "site"), diskCapacity: capacity) === other) + } + + /// `shared` hands out a fresh instance the moment the last one is released, while that + /// one's `deinit` may still hold the file to checkpoint its WAL. Without a busy timeout the + /// reopen fails on that lock every time — 200 runs out of 200 — and caches the failure. + @Test("shared reopens a file while its last instance is still closing") + func sharedReopensAFileWhileItCloses() async throws { + let capacity = Measurement(value: 1, unit: .mebibytes) + for _ in 0..<20 { + let directory = URL.randomTemporaryDirectory + let closing = ReleasableStore(SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity)) + // Enough in the WAL that checkpointing it on close takes a moment. + for index in 0..<50 { + try closing.store?.put( + key: "\(index)", + storageDate: referenceDate, + metadata: Data(), + value: Data(repeating: 1, count: 4096) + ) + } + + async let released: Void = Task.detached { closing.release() }.value + async let reopened = Task.detached { + try SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity).get(key: "0") + }.value + await released + #expect(try await reopened != nil) + } + } +} + +/// Holds the only reference to a store until `release()`, so a test can drop it from another task. +private final class ReleasableStore: @unchecked Sendable { + private let lock = NSLock() + private var held: SQLiteKVCache? + + init(_ store: SQLiteKVCache) { + held = store + } + + var store: SQLiteKVCache? { lock.withLock { held } } + + func release() { + lock.withLock { held = nil } + } } diff --git a/ios/Tests/GutenbergKitTests/TestHelpers.swift b/ios/Tests/GutenbergKitTests/TestHelpers.swift index 2b73716b9..618abf431 100644 --- a/ios/Tests/GutenbergKitTests/TestHelpers.swift +++ b/ios/Tests/GutenbergKitTests/TestHelpers.swift @@ -1,7 +1,22 @@ import Foundation +import Testing @testable import GutenbergKit +/// Polls `condition` until it holds, failing the test at the caller's line if it hasn't within +/// `timeout`. +func waitUntil( + timeout: Duration = .seconds(10), + sourceLocation: SourceLocation = #_sourceLocation, + _ condition: () -> Bool +) async throws { + let deadline = ContinuousClock.now + timeout + while !condition() && ContinuousClock.now < deadline { + try await Task.sleep(for: .milliseconds(10)) + } + try #require(condition(), "timed out waiting", sourceLocation: sourceLocation) +} + func jsonResource(named name: String) throws -> Data { let url = Bundle.module.url(forResource: name, withExtension: "json")! return try Data(contentsOf: url)