Conversation
5 tasks
jkmassel
force-pushed
the
jkmassel/dependency-fetch-sharing
branch
from
September 18, 2026 18:41
320237e to
4de0463
Compare
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/701")Built from e2dfdfe |
jkmassel
force-pushed
the
jkmassel/dependency-fetch-cancelled
branch
from
September 18, 2026 21:32
9908860 to
d8c1d52
Compare
jkmassel
force-pushed
the
jkmassel/dependency-fetch-sharing
branch
from
September 18, 2026 21:32
4de0463 to
0a38ecc
Compare
Follow-ups to keeping the fetch running, from reviewing this PR: - 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 sets 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. 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. Two builds of one manifest can no longer race into it through `copy(to:)`. 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. An editor opened mid-prefetch now joins the settings, theme, site settings, post types, and bundle build already in flight, and fetches only its own post. 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`. `theInFlightFetchKeepsTheEditorAlive` flips to `releasingTheEditorMidFetchFreesIt`: against the previous commit the editor is still alive after 2s; it now passes in 0.36s. `buildBundlePublishesNothingWhenCancelled` fails against the old `buildBundle` with the cancelled bundle on disk, and both new `EditorURLCacheTests` fail against one store per cache with the cached open failure. Mutation-tested: 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.
jkmassel
force-pushed
the
jkmassel/dependency-fetch-cancelled
branch
from
September 18, 2026 21:35
d8c1d52 to
4d5a495
Compare
jkmassel
force-pushed
the
jkmassel/dependency-fetch-sharing
branch
from
September 18, 2026 21:35
0a38ecc to
e2dfdfe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #651, whose review this came out of.
Summary
Why?
We're latency-sensitive on launch — the editor should be ready by the time the loading animation ends. Four things on the async dependency fetch stood in the way.
1. The fetch held its editor
#651 stopped cancelling the fetch when the editor is covered, which left it with no cancellation point at all. The task body,
await self?.prepareEditor(), holds a strongselfacross every suspension inside the call, so a host that released the editor mid-fetch didn't free it — or itsWKWebView— until the fetch ended. Then the full load tail (bundle provider, upload server bind,loadFileURL) ran on a controller nobody held. On a slow network, that can be minutes.2. A cancelled bundle build was saved
EditorAssetLibrary.buildBundleswallows every per-asset failure, cancellation included, then saves the bundle unconditionally.readAssetBundles()reads only the manifest, so a cancelled build left a manifest-only bundle that every later launch served. WordPress-iOS can reach this today:EditorDependencyManager._invalidatecancels an in-flight prefetch and purges without waiting for it.3. Two caches on one site file broke each other
Every
EditorServicebuilds its ownEditorURLCache, and each opened its ownSQLiteKVCacheon the site'seditorurlcache.sqlite— which the store's own docs call undefined behavior. It's worse than contention:connection()caches its result, failure included, for the life of the instance, and nothing sets a busy timeout.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. A broken cache failsprepare()outright — a read error isn't a network error, so the fallback doesn't apply. Not reproduced in WordPress-iOS itself.4. Nothing was shared in flight
Everything site-level is cached on disk once fetched, but an editor opened mid-prefetch repeated the prefetch's requests and its bundle build, splitting the bandwidth the prefetch needed.
What We Explored
1. Capture the service instead of
self❌Hoist
editorServiceinto a local so the task never reaches throughselfacross theawait. It works, but the invariant lives in a capture list inside a 1,200-line view controller withselfin scope on every line — the nextawait self?.…puts the bug back.2. Require
EditorDependencies❌Delete the async path and make the editor a pure function of its inputs. That moves the same await-from-a-view-controller trap into every host.
3. A loader that owns the fetch ✅
The shape
GutenbergEditorControlleralready uses in the same file: the editor owns a helper, and the helper points back through aweakdelegate. The requirements are synchronous, so nothing the loader calls can park the editor mid-call. A first draft started the task ininit, where a baredelegateresolved to the strong parameter rather than the weak property; it only failed to compile because of the?. The task now starts fromfetch(from:), where that parameter isn't in scope.4. Share whole fetches, keyed by configuration ❌
Tried first: one fetch per
EditorConfiguration, with the fields the fetch never reads cleared. It needed a field-by-field analysis of the configuration, a rule about injected clients, andpurge()detaching fetches — and still only joined callers for the same post, because the post ID changes the key.5. Share requests and builds ✅
Key on what goes over the wire and what lands on disk: a request by the request itself plus its session, a build by the directory it writes. Neither needs any configuration analysis, and an editor for any post joins everything site-level.
URLRequest's own==ignorestimeoutInterval,networkServiceType, andhttpBody(measured — the key test caught the timeout gap), so the key compares the first two explicitly and never shares a request with a body.How?
ios/Sources/GutenbergKit/Sources/Services/EditorDependencyLoader.swift: new. Owns the fetch, and reaches the editor only through a
weak let delegatewith synchronous@MainActorrequirements.ios/Sources/GutenbergKit/Sources/EditorViewController.swift: owns its loader and conforms to
EditorDependencyLoaderDelegate.prepareEditor()is replaced by the delegate callbacks, and the fast path moves tostartLoadingEditor(dependencies:), where both flows now end — carrying the fast path's #357 note. The progress view now fades out as the load starts, rather than afterloadEditorreturns.ios/Sources/GutenbergKit/Sources/Helpers/InFlightTasks.swift: new. One task per key, shared by every caller. Cancelling a caller ends only its own wait; the task is cancelled once no caller is left waiting on it.
ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift:
perform(_:)joins an identical request in flight — only safe requests without a body, and only from clients no delegate is watching.EditorHTTPClientis public, so this applies to a host's own GETs through one, too.ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift:
buildBundle(for:)joins a build in flight for the same directory, and refuses to save a cancelled one.ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift:
shared(handle:directory:diskCapacity:)hands every caller the live instance for its file, held weakly so a file no one is using is closed as before.EditorURLCachegoes through it.docs/code/preloading.md: the lifetime and sharing guarantees, for hosts.
Still not shared, correctly: the request for the post itself, and downloads common to two different manifests.
Test Plan
releasingTheEditorMidFetchFreesIt, which replaces fix(ios): keep the dependency fetch running when the editor is covered #651'stheInFlightFetchKeepsTheEditorAlive, fails against fix(ios): keep the dependency fetch running when the editor is covered #651 — the editor is still alive after 2s — and passes in 0.36sbuildBundlePublishesNothingWhenCancelledfails against the oldbuildBundlewith the cancelled bundle on diskEditorURLCacheTestsfail against one store per cache, with the cached open failureEditorDependencyLoaderTestsandInFlightTasksTestsrun on the host, without a simulatorawait, never sharing requests, and keying builds per library rather than per directory are each caughtswift test: 605 + 396 green. iOS Simulatorxcodebuild test -scheme GutenbergKit-Package: 615 + 396 greenmake lint-swiftcleanRelated
Accessibility Testing Instructions
No UI changes beyond when the progress view fades out.