Skip to content

feat!: replace MediaUploadDelegate with an editor-owned MediaProcessor and MediaUploader - #625

Open
jkmassel wants to merge 26 commits into
fix/register-core-media-upload-middlewarefrom
fix/own-media-delegate-strongly
Open

jkmassel wants to merge 26 commits into
fix/register-core-media-upload-middlewarefrom
fix/own-media-delegate-strongly

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Replaces MediaUploadDelegate with two host-facing protocols on both platforms: a MediaProcessor transforms a file before GutenbergKit uploads it, and a MediaUploader performs the whole upload on the host's own stack. On iOS the editor now takes both at init and holds them strongly, so a host that releases its handler mid-upload no longer has the file delivered to WordPress unprocessed.

Stacked on #594. This is the work from #621: the PRs it was split into (#682 through #689, and #651) merged into this branch, so it is reviewed here as one change. #669 is stacked on top.

Summary

  • Hold the media handlers strongly on iOS and take them at init, as Android already does with a plain property.
  • Add stopMediaHandling() for the retain cycle that ownership makes possible, and withdraw the upload endpoint from the page whenever the server stops.
  • Add MediaUploader and remove MediaUploadDelegate.uploadFile, so either GutenbergKit or the host owns an upload and its retries — never half each.
  • Rename MediaUploadDelegate to MediaProcessor, and drop the class requirement from both protocols on iOS.
  • Fail fast when a mediaUploader is supplied without usable site credentials, on both platforms, instead of silently never calling it.
  • Stop cancelling the dependency fetch when the editor is covered (iOS), which ended a still-loading editor's load for good.

Why

A released delegate changed the answer mid-request

The upload server consults the host's handler more than once per request: handlesFile before it copies the upload to disk, then processFile afterwards. iOS held the handler weakly, so a host releasing it between those two reads changed the answer — a file admitted for processing was forwarded untouched. processesForHostReleasedProcessor reproduces it; against a weakly-held handler it fails with passthroughUploadCalled.

Holding it strongly makes the reads agree by construction. That is a trade rather than a free fix: weak also ruled out a retain cycle, and strong does not. A host object that owns the editor and is also its processor forms host → editor → processor → host, deinit never runs, and every editor opened strands a bound loopback NWListener. Most of the iOS work below exists to pay for that.

uploadFile split one upload across two owners

A host implementing uploadFile performed the POST /wp/v2/media and returned the raw response. The editor then drove the post-process retries and the cleanup of a failed attachment behind it — through the WebView, not the host's stack. The hook also received no form fields, so an attachment it uploaded was never attached to its post. Neither is fixable while the hook returns a raw response.

How it works

1. Ownership and teardown (iOS)

  • mediaProcessor and mediaUploader are init parameters, and public private(set). A handler only ever took effect if it was in place before the editor loaded, which a precondition on the setter enforced at runtime. Taking it at construction removes that failure instead of catching it.
  • stopMediaHandling() stops the server, drops the editor's references, and withdraws the endpoint from the page. It is terminal, and only needed by a host whose handler holds the editor back; deinit does the same work for everyone else.
  • Stopping the server clears the endpoint from the page. The port and token are injected at document start, and the upload middleware does not fall back to the default path when a native upload fails, so a stopped server left every image insert failing with a connection error. Both copies are cleared: the live page, and the injected user script that would otherwise restore the dead port at the next document start.
  • HTTPServer.stop() clears the listener's newConnectionHandler. cancel() alone left the final release to Network.framework's queue, and a handler reached through EditorViewController.deinit deallocated off the main thread in 46 of 50 measured runs. Release is now synchronous for an idle server. A request in flight holds its own reference until it unwinds, which the docs on the property, on stopMediaHandling() and on HTTPServer all state.
  • Delivery checks for cancellation first, so a torn-down editor does not put bytes on the wire. The server cannot leave this to the HTTP client: URLSessionProtocol is public, and a conformance that wraps a completion handler has no cancellation awareness. Android gains the same check before it calls a host uploader.
  • Neither protocol is class-bound, so a host can conform with a struct that captures only what the work needs. A value type is not protection by itself — a struct that stores the view controller forms the same cycle — and the docs say so.
  • A DEBUG-only counter logs a fault at four live upload servers, naming the handler types. A deinit assertion cannot report this cycle, because the cycle is what stops deinit from running.

2. Processor or uploader (iOS and Android)

  • A MediaProcessor only changes bytes. GutenbergKit delivers the result and keeps the retries and cleanup.
  • A MediaUploader owns delivery end to end. It receives a MediaUpload — the file, its metadata, the editor's non-file form fields in order (post among them) and the request query — and returns the finished attachment JSON or throws. The host runs its own post-process recovery and deletes its own failed attachment; the protocol docs carry the recipe, including the action parameter core requires.
  • With both supplied, the processor runs first and the uploader delivers. A file the processor declines in handlesFile still reaches the uploader, unprocessed.
  • Media deletes always go through GutenbergKit's own client to the configured site, whoever uploaded the attachment.

3. Credentials

An uploader's deletes need the site root and auth header, so an uploader without them is a configuration that cannot work. iOS traps in init and Android throws from the mediaUploader setter, which puts the host's own line in the stack trace. A processor without credentials is not an error: the server stays down and uploads take the WebView path.

The two platforms' checks had diverged. Android tested only for an empty root, so example.com/wp-json/ started a server whose every relayed delete threw. Both now require a scheme and a host, and the two test suites assert matching cases.

Android hosts must also permit cleartext to localhost, or the handlers are never called. docs/integration.md now documents the entry, and why the library cannot ship it.

4. The dependency fetch survives the editor being covered (iOS)

viewDidDisappear cancelled the async dependency fetch. That callback fires when the editor is merely covered — a modal, a push, a tab switch — and nothing restarts the fetch, so the editor showed the load-error screen and stayed there. The cancellation is removed rather than moved to deinit: the fetch task holds the editor strongly while it runs, so deinit is unreachable until there is nothing left to cancel.

5. Internal

  • GutenbergKitHTTP gains an HTTPRequestHandler protocol and a start overload that takes one. The closure overload is unchanged. MediaUploadServer is now served from a Handler struct with stored dependencies, instead of static functions passing a context parameter through every call.
  • DefaultMediaUploader is renamed InternalMediaClient on both platforms. It is GutenbergKit's HTTP client for the configured site, not a default implementation of a host protocol.

What we explored

  • Keep weak and read the handler once per request (raised in review). It makes the reads agree, but leaves nil ambiguous between "never configured" and "the host released it", frees a handler constructed inline at the call site immediately, and cannot hold a value-type conformer.
  • Detect teardown instead of asking the host to call stopMediaHandling(). UIKit can report it: an ancestor walk, plus a check at viewDidDisappear that the editor has no parent, presenter or window, fired on every dismissal and pop across fourteen hosting shapes, with no false positive on the editor being covered. What it cannot report is whether a detachment is permanent. The call is terminal, so guessing wrong disables media in an editor that survived.
  • Cancel and restart the dependency fetch, or gate the cancellation on isBeingDismissed/isMovingFromParent. A restart would have to be idempotent and not race the fetch already in flight. The flags read false on the editor because hosts install it as a child view controller, so the gate would never fire.

Breaking changes

Before After
MediaUploadDelegate MediaProcessor — conformances need only the new name
editor.mediaUploadDelegate = x (iOS) EditorViewController(configuration:…, mediaProcessor: x)
view.mediaUploadDelegate = x (Android) view.mediaProcessor = x
uploadFile on the delegate Conform to MediaUploader; pass mediaUploader: (iOS) or set view.mediaUploader (Android)
MediaUploadResponse is public Internal — uploadFile was the only public API that named it

A code search of WordPress-iOS and WordPress-Android finds no reference to mediaUploadDelegate. WordPress-iOS's two GutenbergKit.EditorViewController(...) call sites use argument labels, so the added parameters are source-compatible there.

Android needs no counterpart to stopMediaHandling(): a tracing GC collects the cycle ARC cannot, and onDetachedFromWindow already stops the server.

Test plan

  • iOS Simulator xcodebuild test (iPhone 18 Pro, iOS 27.0): 598 + 396 tests pass, including the UIKit-gated teardown and lifecycle suites the host run compiles out
  • Host swift test: 587 + 396 tests pass, including the two exit tests that assert the uploader-credentials trap itself (they run only on macOS)
  • Android :Gutenberg:testDebugUnitTest: 741 tests pass
  • processesForHostReleasedProcessor fails against a weakly-held handler with passthroughUploadCalled
  • coveringTheEditorDoesNotCancelTheDependencyFetch fails against the old viewDidDisappear with a cancelled request
  • uploadServerStartsForAnyHandler runs for processor-only, uploader-only and both; restoring a post-bind check that looks at the processor alone fails only the uploader-only case
  • Ownership assertions are mutation-checked: a server built with no processor fails retainsProcessorForServerLifetime and stopReleasesProcessorThatRetainsTheServer, and dropping mediaProcessor = nil from stopMediaHandling() fails stopMediaHandlingBreaksTheOwnershipCycle
  • Re-imposing the class bound fails to compile: non-class type 'ValueTypeProcessor' cannot conform to class protocol 'MediaProcessor'
  • iOS demo app, with native media upload left on: insert a photo wider than 2000px into an Image block. The block shows the image with no upload error, and the attachment in the site's media library is 2000px on its longest side
  • Android demo app, same steps: same result

Two paths are deliberately untested: the cancellation checks before delivery, and the guard that discards a server which finishes binding after stopMediaHandling(). Each needs teardown driven between two points of a live in-flight request, and a timing-based approximation would pass whether or not it reached the window.

@wpmobilebot

wpmobilebot commented Sep 5, 2026 •

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/625")

Built from 685d3b9

@jkmassel
jkmassel force-pushed the fix/own-media-delegate-strongly branch 2 times, most recently from f15a637 to 9944c98 Compare September 8, 2026 17:43
@jkmassel
jkmassel marked this pull request as ready for review September 8, 2026 20:23

@dcalhoun dcalhoun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this type of design decision is worth @crazytonyli's review.

The alignment with Android and proposed changes make sense. I tested these changes in the Demo app using #629.

I captured findings by Claude, which I believe are legitimate and worth considering.

// the failure actually being hit — not an oversight. #630 drops the class
// requirement from the protocol so a host can conform with a value type.
// swiftlint:disable:next weak_delegate
public var mediaUploadDelegate: (any MediaUploadDelegate)? {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding from Claude:

Is editor ownership a goal here, or only the fix for the mid-request read?

If it's only the fix, reading the delegate once at the top of handleUpload and passing it down keeps the whole request on one delegate while the property stays weak. If it's a goal (assign-and-forget, value-type conformers in #630, Android parity), it brings the cycle from Known issue, and #630 and #631 already build on it.

If the editor keeps ownership

  • A host that hits the cycle has no way out. The setter traps after load, and the server holds its own reference, so clearing the property wouldn't break the cycle anyway. Is the doc warning enough, or is an explicit teardown worth adding?
  • releaseConnectionHandler doesn't cover an in-flight upload. The task holds the delegate until processFile returns, so when the editor is its only owner (as the docs invite), the last release can still land off main. Worth noting on the property, and scoping releaseConnectionHandler's doc to an idle server.

If we read once per request

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ownership is the goal, not only the fix for the mid-request read. The handler is now supplied at init and held for the editor's lifetime, so both points under "if the editor keeps ownership" are addressed:

  • Way out: 733a2a88 adds stopMediaHandling() — it stops the server, drops the editor's references, and withdraws the endpoint from the page. The setter and its after-load trap are gone. stopMediaHandlingBreaksTheOwnershipCycle pins it.
  • In-flight release: b6cd4f27 documents that a request in flight holds its own reference until processFile returns, so the last release can land on the task's executor. That's on the property, on stopMediaHandling(), and on releaseConnectionHandler, whose doc is now scoped to an idle server.

Read-once with weak was rejected: it makes the three reads agree, but leaves nil ambiguous between "never configured" and "host released it", deallocates an init-injected processor immediately, and can't hold the value-type conformers MediaProcessor now allows.

Comment on lines +326 to +328
/// lands there rather than wherever `stop()` was called. For GutenbergKit's
/// upload server that means a host's media handler could be deallocated off the
/// main thread on a path that started in `EditorViewController.deinit`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding from Claude:

Nit: GutenbergKitHTTP doesn't otherwise know about its consumers, and the preceding sentence already covers the effect.

Suggested change
/// lands there rather than wherever `stop()` was called. For GutenbergKit's
/// upload server that means a host's media handler could be deallocated off the
/// main thread on a path that started in `EditorViewController.deinit`.
/// lands there rather than wherever `stop()` was called.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in b6cd4f27 — the paragraph now ends at "wherever stop() was called", followed by the idle-server scoping.

jkmassel added a commit that referenced this pull request Sep 14, 2026
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload
can't lose its handler mid-request. That leaves a host object which holds the
editor back closing a cycle ARC cannot break, and the host has no way out of
it: dropping its own reference wouldn't help, because a running server holds
one of its own through `editor -> uploadServer -> HTTPServer -> listener ->
newConnectionHandler -> handler -> processor`. The only release point was
`deinit` — exactly what a cycle prevents — so every leaked editor also
stranded a bound loopback `NWListener` with a live token, one per post opened.

`releaseMediaHandling()` drops the server and both handlers, and
`viewDidDisappear` calls it once the editor is genuinely going away
(`isBeingDismissed || isMovingFromParent`, both false when a view controller
is merely presented over it — which is why stopping on a bare disappear
previously left uploads broken on return). Both edges have to go: releasing
only one leaves the cycle routed through the other.

This mirrors Android, which tears the server down in `onDetachedFromWindow`.
That — a lifecycle callback rather than a reachability event — is the actual
asymmetry between the platforms, not the garbage collector.

Both handlers move into `init` and become `public private(set)`. They only
take effect if they are in place before the editor loads, a contract that used
to be enforced at runtime by a `precondition` on each setter; taking them at
construction makes that failure unrepresentable rather than caught, so
`hasStartedLoading` and the fail-fast go with it. Android keeps its settable
property and `check(...)` because a `View` is inflated, not constructed by the
host. Constructor injection doesn't close the cycle — a host that owns the
editor and is its own processor writes the same shape — so the release above
is still what opens it.

`deinit` stays as a backstop for an editor that is never presented; that case
gets no lifecycle callback and still leaks, which the docs say plainly.

Also restores `mediaProcessor`'s doc comment, which had been merged into
`mediaUploader`'s as a single stranded block, leaving the primary public
extension point undocumented.

BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable;
pass them to `EditorViewController.init` instead.
jkmassel added a commit that referenced this pull request Sep 14, 2026
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload
can't lose its handler mid-request. That leaves a host object which holds the
editor back closing a cycle ARC cannot break, and the host has no way out of
it: dropping its own reference wouldn't help, because a running server holds
one of its own through `editor -> uploadServer -> HTTPServer -> listener ->
newConnectionHandler -> handler -> processor`. The only release point was
`deinit` — exactly what a cycle prevents — so every leaked editor also
stranded a bound loopback `NWListener` with a live token, one per post opened.

`releaseMediaHandling()` drops the server and both handlers, and
`viewDidDisappear` calls it once the editor is genuinely going away
(`isBeingDismissed || isMovingFromParent`, both false when a view controller
is merely presented over it — which is why stopping on a bare disappear
previously left uploads broken on return). Both edges have to go: releasing
only one leaves the cycle routed through the other.

This mirrors Android, which tears the server down in `onDetachedFromWindow`.
That — a lifecycle callback rather than a reachability event — is the actual
asymmetry between the platforms, not the garbage collector.

Both handlers move into `init` and become `public private(set)`. They only
take effect if they are in place before the editor loads, a contract that used
to be enforced at runtime by a `precondition` on each setter; taking them at
construction makes that failure unrepresentable rather than caught, so
`hasStartedLoading` and the fail-fast go with it. Android keeps its settable
property and `check(...)` because a `View` is inflated, not constructed by the
host. Constructor injection doesn't close the cycle — a host that owns the
editor and is its own processor writes the same shape — so the release above
is still what opens it.

`deinit` stays as a backstop for an editor that is never presented; that case
gets no lifecycle callback and still leaks, which the docs say plainly.

Also restores `mediaProcessor`'s doc comment, which had been merged into
`mediaUploader`'s as a single stranded block, leaving the primary public
extension point undocumented.

BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable;
pass them to `EditorViewController.init` instead.
jkmassel added a commit that referenced this pull request Sep 14, 2026
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload
can't lose its handler mid-request. That leaves a host object which holds the
editor back closing a cycle ARC cannot break, and the host has no way out of
it: dropping its own reference wouldn't help, because a running server holds
one of its own through `editor -> uploadServer -> HTTPServer -> listener ->
newConnectionHandler -> handler -> processor`. The only release point was
`deinit` — exactly what a cycle prevents — so every leaked editor also
stranded a bound loopback `NWListener` with a live token, one per post opened.

`tearDown()` stops the server and drops both handlers, opening the cycle from
the editor's side. It is the host's call to make, because the editor cannot
detect its own teardown. An earlier revision of this branch tried to infer it
from `viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`.
That does not work: `isBeingDismissed` does not propagate down a containment
chain, and the editor is a child view controller in every real host, so both
flags read false on it while an ancestor carries the true value. Measured on a
simulator across four presentation shapes, the SwiftUI demo, and the real
editor — the guard blocked the release every time. WordPress-iOS hit the same
UIKit behaviour and carries `isBeingDismissedDirectlyOrByAncestor()` for it;
walking ancestors would fix those shapes and still misfire on containers that
re-parent (`UIPageViewController` recycling a child sets `isMovingFromParent`
while the editor survives) and stay silent when a stack is reset out from
under a covered editor. A heuristic over host configurations we cannot
enumerate is the wrong trade for a library, so this states the contract
instead.

Both handlers also move into `init` and become `public private(set)`. They
only take effect if they are in place before the editor loads, a contract
that used to be enforced at runtime by a `precondition` on each setter;
taking them at construction makes that failure unrepresentable rather than
caught, so `hasStartedLoading` and the fail-fast go with it. Android keeps its
settable property and `check(...)` because a `View` is inflated, not
constructed by the host.

`deinit` is unchanged and remains the ordinary path: with no cycle, ARC
releases the handlers and `deinit` stops the server, so a host that follows
the documented rule — don't retain the editor from your handler — needs
nothing. Verified in the demo app, where `deinit` runs on close.

Also restores `mediaProcessor`'s doc comment, which had been merged into
`mediaUploader`'s as a single stranded block, leaving the primary public
extension point undocumented.

BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable;
pass them to `EditorViewController.init` instead.
jkmassel added a commit that referenced this pull request Sep 15, 2026
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload
can't lose its handler mid-request. That leaves a host object which holds the
editor back closing a cycle ARC cannot break, and the host has no way out of
it: dropping its own reference wouldn't help, because a running server holds
one of its own through `editor -> uploadServer -> HTTPServer -> listener ->
newConnectionHandler -> handler -> processor`. The only release point was
`deinit` — exactly what a cycle prevents — so every leaked editor also
stranded a bound loopback `NWListener` with a live token, one per post opened.

`stopMediaHandling()` stops the server and drops both handlers, opening the
cycle from the editor's side. It is the host's call to make, because the
editor cannot detect its own teardown. An earlier revision inferred it from
`viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`; that
does not work, because `isBeingDismissed` does not propagate down a
containment chain and the editor is a child view controller in every real
host. Measured across four presentation shapes, the SwiftUI demo, and the real
editor: the guard blocked the release every time. Walking ancestors fixes
those shapes but still misfires where a container re-parents, and stays silent
when a stack is reset under a covered editor. No UIKit callback distinguishes
teardown from being covered or re-parented, so this states the contract rather
than guessing at it.

Stopping now also withdraws the endpoint from the page. The port and token are
injected once at document start, and `nativeMediaUploadMiddleware` refuses to
retry a failed native upload directly, on the stated assumption that an
advertised port is a reachable one ("cleared on stop"). Nothing cleared it, so
a stopped server left every image insert failing with a connection error on a
working connection — and an uploader host's orphan-cleanup DELETE broken with
it. `revokeNativeUploadEndpoint()` clears the live page, the `localStorage`
copy `getGBKit()` falls back to, and the injected user script that would
otherwise restore the dead port at the next document start, including the
reload that recovers a terminated WebContent process. Uploads fall back to the
default WebView path instead of failing.

`MediaProcessor` and `MediaUploader` drop their `AnyObject` constraint,
matching `HTTPRequestHandler` in GutenbergKitHTTP, which documents the reason:
a value type cannot participate in a reference cycle, so the question doesn't
arise. It doesn't prevent the cycle — a struct holding a class reference
closes it just as well — but it makes the acyclic shape expressible, and an
`AnyObject` protocol taken at `init` reads like an invitation to pass `self`.
Source-compatible: every existing class conformer still conforms.

A DEBUG-only census counts live upload servers and logs a fault past four.
Each live server is a bound loopback listener, one per editor, so monotone
growth is this cycle and nothing else produces it — `warmup()` passes no
handlers and starts no server. It is the only detectable symptom available: a
`deinit` assertion cannot fire, because a cycle is what stops `deinit` from
running. Logged, never fatal; the threshold is a heuristic, and crashing a
host's debug build over a heuristic is a worse trade than the leak.

Both handlers also move into `init` and become `public private(set)`. They
only take effect if they are in place before the editor loads, a contract that
used to be enforced at runtime by a `precondition` on each setter; taking them
at construction makes that failure unrepresentable rather than caught, so
`hasStartedLoading` and the fail-fast go with it. The retention rule now lives
on the `init` parameters and in the integration guide, since the initializer
is the only media call site a host writes.

`deinit` is unchanged and remains the ordinary path: with no cycle, ARC
releases the handlers and `deinit` stops the server.

BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable;
pass them to `EditorViewController.init` instead.
jkmassel added a commit that referenced this pull request Sep 15, 2026
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload
can't lose its handler mid-request. That leaves a host object which holds the
editor back closing a cycle ARC cannot break, and the host has no way out of
it: dropping its own reference wouldn't help, because a running server holds
one of its own through `editor -> uploadServer -> HTTPServer -> listener ->
newConnectionHandler -> handler -> processor`. The only release point was
`deinit` — exactly what a cycle prevents — so every leaked editor also
stranded a bound loopback `NWListener` with a live token, one per post opened.

`stopMediaHandling()` stops the server and drops both handlers, opening the
cycle from the editor's side. It is the host's call to make, because the
editor cannot detect its own teardown. An earlier revision inferred it from
`viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`; that
does not work, because `isBeingDismissed` does not propagate down a
containment chain and the editor is a child view controller in every real
host. Measured across four presentation shapes, the SwiftUI demo, and the real
editor: the guard blocked the release every time. Walking ancestors fixes
those shapes but still misfires where a container re-parents, and stays silent
when a stack is reset under a covered editor. No UIKit callback distinguishes
teardown from being covered or re-parented, so this states the contract rather
than guessing at it.

Stopping now also withdraws the endpoint from the page. The port and token are
injected once at document start, and `nativeMediaUploadMiddleware` refuses to
retry a failed native upload directly, on the stated assumption that an
advertised port is a reachable one ("cleared on stop"). Nothing cleared it, so
a stopped server left every image insert failing with a connection error on a
working connection — and an uploader host's orphan-cleanup DELETE broken with
it. `revokeNativeUploadEndpoint()` clears the live page, the `localStorage`
copy `getGBKit()` falls back to, and the injected user script that would
otherwise restore the dead port at the next document start, including the
reload that recovers a terminated WebContent process. Uploads fall back to the
default WebView path instead of failing.

`MediaProcessor` and `MediaUploader` drop their `AnyObject` constraint,
matching `HTTPRequestHandler` in GutenbergKitHTTP, which documents the reason:
a value type cannot participate in a reference cycle, so the question doesn't
arise. It doesn't prevent the cycle — a struct holding a class reference
closes it just as well — but it makes the acyclic shape expressible, and an
`AnyObject` protocol taken at `init` reads like an invitation to pass `self`.
Source-compatible: every existing class conformer still conforms.

A DEBUG-only census counts live upload servers and logs a fault past four.
Each live server is a bound loopback listener, one per editor, so monotone
growth is this cycle and nothing else produces it — `warmup()` passes no
handlers and starts no server. It is the only detectable symptom available: a
`deinit` assertion cannot fire, because a cycle is what stops `deinit` from
running. Logged, never fatal; the threshold is a heuristic, and crashing a
host's debug build over a heuristic is a worse trade than the leak.

Both handlers also move into `init` and become `public private(set)`. They
only take effect if they are in place before the editor loads, a contract that
used to be enforced at runtime by a `precondition` on each setter; taking them
at construction makes that failure unrepresentable rather than caught, so
`hasStartedLoading` and the fail-fast go with it. The retention rule now lives
on the `init` parameters and in the integration guide, since the initializer
is the only media call site a host writes. Both also spell out what reuse
across editor sessions requires: the editor drops only its own reference when
it goes, so a host sharing one handler — the expected shape for an uploader,
whose background session or offline queue outlives any editor — has to keep
its own. Sharing is the safer shape, since a handler owned by something
longer-lived than any editor is a leaf and cannot form the cycle at all.

`deinit` is unchanged and remains the ordinary path: with no cycle, ARC
releases the handlers and `deinit` stops the server.

BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable;
pass them to `EditorViewController.init` instead.
jkmassel added a commit that referenced this pull request Sep 15, 2026
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload
can't lose its handler mid-request. That leaves a host object which holds the
editor back closing a cycle ARC cannot break, and the host has no way out of
it: dropping its own reference wouldn't help, because a running server holds
one of its own through `editor -> uploadServer -> HTTPServer -> listener ->
newConnectionHandler -> handler -> processor`. The only release point was
`deinit` — exactly what a cycle prevents — so every leaked editor also
stranded a bound loopback `NWListener` with a live token, one per post opened.

`stopMediaHandling()` stops the server and drops both handlers, opening the
cycle from the editor's side. It is the host's call to make, because the
editor cannot detect its own teardown. An earlier revision inferred it from
`viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`; that
does not work, because `isBeingDismissed` does not propagate down a
containment chain and the editor is a child view controller in every real
host. Measured across four presentation shapes, the SwiftUI demo, and the real
editor: the guard blocked the release every time. Walking ancestors fixes
those shapes but still misfires where a container re-parents, and stays silent
when a stack is reset under a covered editor. No UIKit callback distinguishes
teardown from being covered or re-parented, so this states the contract rather
than guessing at it.

Stopping now also withdraws the endpoint from the page. The port and token are
injected once at document start, and `nativeMediaUploadMiddleware` refuses to
retry a failed native upload directly, on the stated assumption that an
advertised port is a reachable one ("cleared on stop"). Nothing cleared it, so
a stopped server left every image insert failing with a connection error on a
working connection — and an uploader host's orphan-cleanup DELETE broken with
it. `revokeNativeUploadEndpoint()` clears the live page, the `localStorage`
copy `getGBKit()` falls back to, and the injected user script that would
otherwise restore the dead port at the next document start, including the
reload that recovers a terminated WebContent process. Uploads fall back to the
default WebView path instead of failing.

`MediaProcessor` and `MediaUploader` drop their `AnyObject` constraint,
matching `HTTPRequestHandler` in GutenbergKitHTTP, which documents the reason:
a value type cannot participate in a reference cycle, so the question doesn't
arise. It doesn't prevent the cycle — a struct holding a class reference
closes it just as well — but it makes the acyclic shape expressible, and an
`AnyObject` protocol taken at `init` reads like an invitation to pass `self`.
Source-compatible: every existing class conformer still conforms.

A DEBUG-only census counts live upload servers and logs a fault past four.
Each live server is a bound loopback listener, one per editor, so monotone
growth is this cycle and nothing else produces it — `warmup()` passes no
handlers and starts no server. It is the only detectable symptom available: a
`deinit` assertion cannot fire, because a cycle is what stops `deinit` from
running. Logged, never fatal; the threshold is a heuristic, and crashing a
host's debug build over a heuristic is a worse trade than the leak.

Both handlers also move into `init` and become `public private(set)`. They
only take effect if they are in place before the editor loads, a contract that
used to be enforced at runtime by a `precondition` on each setter; taking them
at construction makes that failure unrepresentable rather than caught, so
`hasStartedLoading` and the fail-fast go with it. The retention rule now lives
on the `init` parameters and in the integration guide, since the initializer
is the only media call site a host writes. Both also spell out what reuse
across editor sessions requires: the editor drops only its own reference when
it goes, so a host sharing one handler — the expected shape for an uploader,
whose background session or offline queue outlives any editor — has to keep
its own. Sharing is the safer shape, since a handler owned by something
longer-lived than any editor is a leaf and cannot form the cycle at all.

The rule is stated as what it actually is. Nothing in GutenbergKit hands a
handler the editor — every value crossing that boundary is a `Sendable` value
type — so the cycle is entirely host-authored: it forms only when the object
that already holds the editor to drive it also conforms. "Don't conform the
object that owns this editor" is the actionable phrasing, and it costs the
host nothing, because these methods are called off the main actor and could
not have reached that object's state anyway.

`deinit` is unchanged and remains the ordinary path: with no cycle, ARC
releases the handlers and `deinit` stops the server.

BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable;
pass them to `EditorViewController.init` instead.
jkmassel added a commit that referenced this pull request Sep 15, 2026
#625 made `mediaProcessor` and `mediaUploader` strong so an in-flight upload
can't lose its handler mid-request. That leaves a host object which holds the
editor back closing a cycle ARC cannot break, and the host has no way out of
it: dropping its own reference wouldn't help, because a running server holds
one of its own through `editor -> uploadServer -> HTTPServer -> listener ->
newConnectionHandler -> handler -> processor`. The only release point was
`deinit` — exactly what a cycle prevents — so every leaked editor also
stranded a bound loopback `NWListener` with a live token, one per post opened.

`stopMediaHandling()` stops the server and drops both handlers, opening the
cycle from the editor's side. It is the host's call to make, because the
editor cannot detect its own teardown. An earlier revision inferred it from
`viewDidDisappear` gated on `isBeingDismissed || isMovingFromParent`; that
does not work, because `isBeingDismissed` does not propagate down a
containment chain and the editor is a child view controller in every real
host. Measured across four presentation shapes, the SwiftUI demo, and the real
editor: the guard blocked the release every time. Walking ancestors fixes
those shapes but still misfires where a container re-parents, and stays silent
when a stack is reset under a covered editor. No UIKit callback distinguishes
teardown from being covered or re-parented, so this states the contract rather
than guessing at it.

Stopping now also withdraws the endpoint from the page. The port and token are
injected once at document start, and `nativeMediaUploadMiddleware` refuses to
retry a failed native upload directly, on the stated assumption that an
advertised port is a reachable one ("cleared on stop"). Nothing cleared it, so
a stopped server left every image insert failing with a connection error on a
working connection — and an uploader host's orphan-cleanup DELETE broken with
it. `revokeNativeUploadEndpoint()` clears the live page, the `localStorage`
copy `getGBKit()` falls back to, and the injected user script that would
otherwise restore the dead port at the next document start, including the
reload that recovers a terminated WebContent process. Uploads fall back to the
default WebView path instead of failing.

`MediaProcessor` and `MediaUploader` drop their `AnyObject` constraint,
matching `HTTPRequestHandler` in GutenbergKitHTTP, which documents the reason:
a value type cannot participate in a reference cycle, so the question doesn't
arise. It doesn't prevent the cycle — a struct holding a class reference
closes it just as well — but it makes the acyclic shape expressible, and an
`AnyObject` protocol taken at `init` reads like an invitation to pass `self`.
Source-compatible: every existing class conformer still conforms.

A DEBUG-only census counts live upload servers and logs a fault past four.
Each live server is a bound loopback listener, one per editor, so monotone
growth is this cycle and nothing else produces it — `warmup()` passes no
handlers and starts no server. It is the only detectable symptom available: a
`deinit` assertion cannot fire, because a cycle is what stops `deinit` from
running. Logged, never fatal; the threshold is a heuristic, and crashing a
host's debug build over a heuristic is a worse trade than the leak.

Both handlers also move into `init` and become `public private(set)`. They
only take effect if they are in place before the editor loads, a contract that
used to be enforced at runtime by a `precondition` on each setter; taking them
at construction makes that failure unrepresentable rather than caught, so
`hasStartedLoading` and the fail-fast go with it. The retention rule now lives
on the `init` parameters and in the integration guide, since the initializer
is the only media call site a host writes. Both also spell out what reuse
across editor sessions requires: the editor drops only its own reference when
it goes, so a host sharing one handler — the expected shape for an uploader,
whose background session or offline queue outlives any editor — has to keep
its own. Sharing is the safer shape, since a handler owned by something
longer-lived than any editor is a leaf and cannot form the cycle at all.

The rule is stated as what it actually is. Nothing in GutenbergKit hands a
handler the editor — every value crossing that boundary is a `Sendable` value
type — so the cycle is entirely host-authored: it forms only when the object
that already holds the editor to drive it also conforms. "Don't conform the
object that owns this editor" is the actionable phrasing, and it costs the
host nothing, because these methods are called off the main actor and could
not have reached that object's state anyway.

`deinit` is unchanged and remains the ordinary path: with no cycle, ARC
releases the handlers and `deinit` stops the server.

BREAKING CHANGE: `mediaProcessor` and `mediaUploader` are no longer settable;
pass them to `EditorViewController.init` instead.
@jkmassel jkmassel changed the title fix(ios): own the media upload delegate instead of holding it weakly fix(ios)!: own the media upload delegate, and give hosts a way out Sep 16, 2026
@jkmassel jkmassel added [Type] Breaking Change For PRs that introduce a change that will break existing functionality and removed [Type] Bug An existing feature does not function as intended labels Sep 16, 2026
jkmassel and others added 22 commits October 1, 2026 20:07
The server reads the delegate three times per request — once at the
admission gate (`handlesFile`), then again for `processFile` and
`uploadFile` — and those reads are separated by a synchronous disk copy
and an unbounded `processFile`. Held weakly, a host that released its
delegate in that window changed the answer between reads: a file admitted
for processing was forwarded to WordPress unprocessed.

Hold it strongly, as Android already does with a plain `val`. Immutable
strong references make the three reads agree by construction, and an
in-flight upload keeps the delegate alive until it unwinds.

The `weak` bought no leak protection to trade away. The cycle it named
runs through `EditorViewController.mediaUploadDelegate` — a host object
retaining the view controller forms `EditorViewController -> delegate ->
EditorViewController` regardless of how this container holds it. What it
did buy was the reference vanishing mid-request.

So `mediaUploadDelegate` becomes strong too, and the machinery that
existed only to police the old contract goes with it:
`mediaUploadDelegateWasAssigned` and the released-before-load trap have
nothing left to catch, because the editor now owns the delegate for its
lifetime. Hosts no longer need to retain it themselves.

`UploadContext` becomes a struct and drops its `@unchecked Sendable`
opt-out: `MediaUploadDelegate` is `Sendable` and `DefaultMediaUploader` is
`@unchecked Sendable`, so it is implicitly Sendable.

`doesNotStronglyRetainDelegate` pinned the invariant being removed, so it
is replaced by `retainsDelegateForServerLifetime`, asserting both halves —
the server owns the delegate while it runs, and releases it afterward.
`processesForHostReleasedDelegate` covers the bug directly; against a weak
container it fails with the real symptom, `passthroughUploadCalled`.

SwiftLint's `weak_delegate` is suppressed with the reasoning inline. The
rule is arguably right that the name no longer fits — a later commit
renames the property, and the suppression goes away with it.
`mediaUploadDelegate` is a strong `var`, which is only safe while the
delegate does not retain the editor back. Assert that the editor still
reaches `deinit` — and releases the delegate it owns — so a cycle
introduced here fails a test instead of leaking silently.

Extracted from the handler-ownership refactor that this branch drops:
the server-side handler object lands further up the stack instead, but
this half of the contract belongs with the change that creates it.
`NWListener.newConnectionHandler` retains the request handler and, through
it, whatever the caller's closure captured — for the upload server, that is
now the host's media delegate. `cancel()` does not drop the block:
Network.framework holds the listener until cancellation completes on its own
queue, so the final release landed there rather than on the thread that
called `stop()`. A delegate reached through `EditorViewController.deinit`
therefore deallocated off the main thread, measured at 46/50 on the
listener's queue — `Timer.invalidate()` and `UIView` teardown in a host's
`deinit` are both unsafe there.

Clearing it after `cancel()` (not before — the listener is already torn
down, so it is never live without a handler) makes teardown synchronous on
the caller's thread. `retainsDelegateForServerLifetime` asserts the release
outright instead of polling a one-second budget for it.
Silences the `#WeakMutability` warning this declaration emitted on every
build — the only warning in the library and test targets.
Both comments claimed the retain cycle is one this code "can neither create
nor prevent". Only the second half was true. Flipping `UploadContext` alone,
with the property left `weak`, closes the ring through `uploadServer` and
leaks the owner — that container's `weak` was its single weak link, so it
demonstrably could prevent a cycle. The property is the same story in mirror
image: strong here is exactly what lets a delegate that retains the editor
back close the shorter ring, and `weak` would rule it out.

Neither point argues against the change — the delegate vanishing mid-request
is the failure that was actually being hit. But justifying it with a claim
that does not hold is how the next investigation into a leaked editor gets
misdirected.
`deinitReleasesEditorAndDelegate` passes unchanged against the pre-PR `weak`
property, and passes with its `mediaUploadDelegate` assignment deleted
outright. `LifetimeProbeDelegate` holds no reference to the editor, so the
cycle the failure message names cannot be constructed in the fixture; and the
test never touches `view`, so `viewDidLoad` never runs, `startUploadServer()`
never runs, and the `UploadContext` this PR changes is never built.

The ownership change is covered by `processesForHostReleasedDelegate` and
`retainsDelegateForServerLifetime`, both of which fail against a weakly-held
delegate with the real symptom. Covering the composite teardown path — editor
loaded, server started, editor deallocated — needs a loaded editor and a real
listener, which is E2E territory.
The editor holds `mediaUploadDelegate` strongly so an in-flight upload can't
lose it mid-request — losing the delegate mid-request was the failure actually
being hit, and `weak` would rule it out. That is a deliberate trade, and this
commit pays the rest of its cost rather than leaving it in the docs.

A host object that owns the editor and is also its delegate closes a cycle ARC
cannot break, so `deinit` never runs and every editor opened strands a bound
loopback `NWListener` with a live token. `stopMediaHandling()` is the way out:
it stops the server, drops the delegate, and withdraws the endpoint from the
page. It has to be the host's call. Not because UIKit can't report a teardown
— an ancestor walk plus an orphan check at `viewDidDisappear` fires correctly
across fourteen hosting shapes, including this editor's shape in WordPress-iOS
— but because it can't report whether a detachment is *permanent*. A host may
re-present the same editor, and since the call is terminal, guessing wrong
disables media in an editor that survived.

Nothing here hands the delegate the editor: every value crossing that boundary
is a value type. So the cycle is entirely host-authored, and the rule is
narrower than "don't retain the editor" — don't conform the object that owns
it. That costs nothing, because `processFile` runs off the main actor and
could not have reached that object's state anyway.

The delegate also moves into `init` and becomes `public private(set)`. It only
takes effect if it is in place before the editor loads, a contract that used to
be enforced at runtime by a `precondition` on the setter; taking it at
construction makes that failure unrepresentable rather than caught, so
`hasStartedLoading` and the fail-fast go with it.

Stopping now also withdraws the endpoint from the page. The port and token are
injected once at document start, and `nativeMediaUploadMiddleware` refuses to
retry a failed native upload directly, on the stated assumption that an
advertised port is a reachable one ("cleared on stop"). Nothing cleared it. A
stopped server left every image insert failing with a connection error on a
working connection. `revokeNativeUploadEndpoint()` clears the live page, the
`localStorage` copy `getGBKit()` falls back to, and the injected user script
that would otherwise restore the dead port at the next document start.

A DEBUG-only census counts live upload servers and logs a fault past four.
Each live server is a bound loopback listener, one per editor, so monotone
growth is this cycle and nothing else produces it — `warmup()` passes no
delegate and starts no server. It is the only detectable symptom: a `deinit`
assertion cannot fire, because a cycle is what stops `deinit` from running.

Answers @dcalhoun's review question on this PR: ownership is the goal, and yes,
an explicit teardown was worth adding.

BREAKING CHANGE: `mediaUploadDelegate` is no longer settable; pass it to
`EditorViewController.init` instead.
Both delivery paths could put bytes on the wire after the editor was gone.
`stopMediaHandling()` and `EditorViewController.deinit` both call `stop()`,
which cancels the in-flight connection tasks, but Swift cancellation is
cooperative: the body read is an uninterruptible loop and a host's
`processFile` need not check at all, so a request can reach delivery well
after teardown. Whether it then actually reached WordPress rested entirely
on URLSession noticing the cancellation.

That is not a guarantee the server can rely on. `URLSessionProtocol` is
public and documented for dependency injection, and the obvious
conformance for a host wrapping a callback-based stack —
`withCheckedThrowingContinuation` around a completion handler — has no
cancellation awareness at all. Such a host would upload deterministically
after teardown, and the response is discarded either way, leaving an
attachment on the site that nothing cleans up.

Check cancellation explicitly before delivery in `processAndUpload` and
before the passthrough forward, so the guarantee comes from this file
rather than from the HTTP client's behavior — and so `stopMediaHandling()`'s
documented "any upload in flight is cancelled" holds for every host, not
just those on a stock `URLSession`. `uploadErrorResponse` already logs
CancellationError quietly, and HTTPServer drops the response for a
cancelled task.

Not covered by a test: reaching the window deterministically means driving
teardown between the parse and the delivery of a live socket request, and
a timing-based approximation would be flaky without pinning the behavior.
`retainsDelegateForServerLifetime` names two properties and only tested one.
The delegate was bound to a strong local for the whole `do` block, so
`#expect(weakDelegate != nil)` was satisfied by that local — the server's
ownership was never what the assertion depended on.

Confirmed by mutation. With `UploadContext(uploadDelegate: nil, ...)`, so the
server holds no reference to the delegate at all, the test **passed**. Nil the
host's reference before the assert — the way `processesForHostReleasedDelegate`
already does — and the same mutation fails it.

The release half was always live and is unchanged: no-op'ing
`releaseConnectionHandler()` still fails the trailing
`#expect(weakDelegate == nil)`, which is the regression 7124457 added it for.

From 8827ba4, earlier in this branch — the commit that introduced the test to
pin the strong-ownership fix it could not actually detect.
…legate

The server holds the delegate strongly for the duration of a request, so a
delegate that holds the server back closes a loop through the listener's
captured blocks: `listener -> newConnectionHandler -> handler -> UploadContext
-> delegate -> server`. Nothing else in the suite covers that edge — the
editor-side tests never start a server, and `retainsDelegateForServerLifetime`
uses a leaf delegate, so its release needs nothing to be broken first.

Both assertions are load-bearing, confirmed by mutation. Building the context
with `uploadDelegate: nil`, so the server holds no reference at all, fails the
first: the delegate is freed as soon as the host's local goes out of scope.

That stopping resolves the loop at all depends on Network.framework behaviour
this package now relies on: for a deployment target of iOS 16 or later (this
package requires 17), cancelling an NWListener releases the blocks it captured
(rdar://89677097, documented in the macOS 13 release notes). No-op'ing
`releaseConnectionHandler()` — the explicit clear added in 7124457 — still
frees the delegate, one poll tick later, which is that behaviour doing the work.
Before it, the blocks were held for the listener's lifetime. Pinned so a
regression, or a lowered deployment target, fails loudly instead of quietly
stranding listeners.
`releaseConnectionHandler()`'s doc claimed teardown releases the handler's
captures on the caller's thread, unqualified. That holds for an idle server. A
request in flight keeps its own copy of what the handler captured until the task
unwinds, so a server stopped mid-request releases last on the task's executor no
matter what the clear does — and when the editor is the delegate's only owner,
that is where the host's `deinit` runs.

Stated on each surface a host reads. The `GutenbergKitHTTP` doc scopes its claim
to an idle server and drops the sentence naming GutenbergKit's upload server —
the package doesn't otherwise know its consumers, and the preceding sentence
already covers the effect. `mediaUploadDelegate` says the release can be late
and off the main thread. `stopMediaHandling()` stops implying that "cancelled"
means "stopped now": cancellation is cooperative, so a `processFile` that
ignores it runs to completion and holds the delegate until it returns.

Both points from @dcalhoun's review of this PR.
`revokeNativeUploadEndpoint()` passed `completionHandler: nil` and used `try?`
on the configuration rebuild, so both of its steps could fail without saying so.

Diagnostics, not a fix — the residual risk is narrow. The two plausible failures
are self-healing: a terminated WebContent process reloads through
`controllerWebContentProcessDidTerminate`, and a page that hasn't loaded yet has
no endpoint to withdraw. Both land on the rebuilt user script, which advertises
no port. What is left is a live page whose eval failed anyway, holding a port
nothing is listening on until the next document start, and a rebuild that threw,
leaving that next document start with no `window.GBKit` at all. Neither should
be invisible, and the second is the more interesting of the two: the load path
lets the same call throw and aborts, so a failure here is strictly quieter than
the one the editor already refuses to ignore.

It can't route through this file's `evaluate(_:isCritical:)` helper, which hands
errors to `handleError` and presents a `UIAlertController`: this runs while the
editor is going away.
`startUploadServer()` assigned `self.uploadServer` after awaiting the bind, so a
`stopMediaHandling()` landing in that window was undone. The server was stored
after the endpoint had already been withdrawn from the page, and because the
context captured the delegate before the await, the cycle the call exists to
open stayed closed — the property was nil, the server's reference was not.

Small window, and nobody is in it today. Binding a loopback listener measures at
or under a millisecond (`startAndStop` reports 0.001s); the five-second
`defaultStartTimeout` is a ceiling for a listener that can't become ready, not a
typical wait. `stopMediaHandling()` is new in this branch and has no callers,
and WordPress-iOS sets no delegate at all. Reaching this needs a host that
adopts the retaining shape the docs discourage and then tears the editor down
inside that millisecond.

Worth four lines anyway. `startUploadServer()` has exactly one call site and
runs at most once per editor, so with the guard, "terminal" is a property of the
code rather than of how fast a listener binds.

Not covered by a test: suspending a real bind mid-flight is the only way into
the window, and a timing-based approximation would pass whether or not it got
there.
`DefaultMediaUploader` reads as an implementation of a host-facing
protocol — the "default" one, as against a host's. It is not. It is
GutenbergKit's own HTTP client for the configured site: it performs the
uploads no host took over, and it relays every media delete, because the
editor only ever asks to delete `/wp/v2/media/<id>` on the configured
site.

Rename it, and the `defaultUploader` parameters and properties that carry
it, on both platforms. Sweep the prose and error strings that used the
retired vocabulary too, including the `UploadContext` doc header and
Android's three media-client messages. The host-facing docs still say
"the default uploader" as a role: `InternalMediaClient` is internal on
both platforms, so naming it in prose a host reads would be worse.

On iOS this also narrows two signatures. `passthroughResponse` and
`handleDelete` took the whole `UploadContext` and touched only the client.
Pass it directly. On the delete path that is more than tidiness: a
deletion always relays to the configured site, never to a delegate. That
was a convention the signature let you break; now the type won't. The
three functions that keep the context genuinely need every field.

Android's server holds the client as a constructor property rather than
threading a context, so it needs the rename only — and because its
`handleDelete` is an instance method with the delegate in scope, the
delete-path convention stays a convention there. The type-level guarantee
is iOS-only.
* feat: add MediaUploader, for a host that owns the whole upload

Performing a media upload — and retrying it — should be a single,
all-or-nothing responsibility: either GutenbergKit performs the upload and
owns its retries, or the host does. Both go to the same configured site;
the only difference is who executes the requests.

`MediaUploadDelegate.uploadFile` doesn't offer that. A host performs the
`POST /wp/v2/media` and returns the raw response it received — then the
editor, reading that response, drives the `post-process` retries and the
orphan cleanup behind it, through the WebView rather than the host's
stack. A host that took over uploads to run them through its own
networking still didn't own the retries. It also receives no form fields,
so an attachment it uploads lands unattached to its post.

Add `MediaUploader`, which owns the upload end to end:

- `upload(_:)` returns the finished attachment or throws. There is no raw
  response left for the editor to retry behind it, so the host drives its
  own post-process recovery and force-deletes its own orphan on terminal
  failure.
- It receives a `MediaUpload` carrying the file, its metadata, the
  editor's non-file form fields (`post`, additionalData) and the request
  query (`?_embed`) — everything needed to reproduce a native request.
- Fields are a `MediaUploadField` list rather than a dictionary, so
  repeated names (a `field[]` array) survive verbatim and in order.

Additive for hosts: `uploadFile` still works and is marked deprecated,
pointing them at the replacement, and an uploader takes precedence when
both are set. Internally the upload server's startup gate widens to admit
an uploader as well as a delegate. GutenbergKit's own build keeps one
deprecation warning at the call site that supports the old hook — the
marker exists to tell hosts to migrate, and supporting the hook until it
is removed means calling it.

With an uploader set, the delegate's metadata gate can no longer decline a
file: the gate exists to skip a temp copy for a file the delegate won't
touch, but an uploader takes over delivery for *every* file, so passing
through would silently bypass it. Covered on both platforms.

`MediaUploadServerTest` crosses Detekt's LargeClass threshold; baselined
rather than split, which is its own change.

* fix: don't hand a declined file to processFile, and close the gaps around it

Follow-ups to the MediaUploader commit: a behavior bug in the metadata gate, a
cross-platform divergence, a missing cancellation check on Android, four
documentation defects, two test gaps, and a shadowed local.

- `processFile` ran on a file the delegate's metadata gate had declined. Widening
  the gate to `uploader != nil || delegateWantsFile` left `processFile` called
  unconditionally, so an image-only delegate paired with an uploader was handed
  the `.mov` it had just said it won't touch — breaking the contract
  `handlesFile` documents. `delegateWantsFile` is now carried into
  `processAndUpload` and gates `processFile`. With an uploader set the file is
  still delivered; it just skips processing on the way.

- iOS evaluated `handlesFile` eagerly while Android's `&&` short-circuited past
  it, so the same host saw one callback per upload on iOS and zero on Android.
  Android now binds it eagerly too: asked exactly once per upload on both.

- Android had no pre-flight cancellation check before handing work to the host
  uploader, where iOS has `Task.checkCancellation()`. Added
  `currentCoroutineContext().ensureActive()`, so a torn-down editor no longer
  starts an upload whose attachment nobody would clean up.

- The recovery recipe omitted `post-process`'s required `action` parameter. Core
  registers `action` as required, so a host following the doc verbatim would 400
  five times and then run the doc's *other* instruction —
  `DELETE /wp/v2/media/<id>?force=true` — destroying an attachment
  `wp_update_image_subsizes()` would have recovered.

- `mediaUploader`'s doc had been appended to `mediaUploadDelegate`'s `///` block,
  merging the two: `mediaUploadDelegate` shipped with no documentation and
  `mediaUploader` opened by describing a delegate. Confirmed with
  `swiftc -emit-symbol-graph` (`mediaUploadDelegate => None`); both now bind
  their own 11 lines.

- `formFields` and `deprecatedUploadFile` were inserted between
  `attachmentId(fromPath:)`'s doc and its declaration — merging into it on iOS,
  dropping it outright on Android — costing the "deliberately narrow, not a
  general REST proxy" rationale. Moved below their only caller, per AGENTS.md's
  call-order rule.

- `ReplaceWith("MediaUploader")` takes a replacement *expression*; applying the
  quick-fix drops all three arguments and leaves a type name where a
  `MediaUploadResponse?` was expected. Removed, with a note so it doesn't return.

- Nothing pinned the Android gate or the uploader/deprecated-hook precedence:
  deleting `&& mediaUploader == null` or reordering the two delivery paths left
  the suite green. Three tests added; both mutations now fail.

- `DecliningDelegate` duplicated the pre-existing `DeclineByMetadataDelegate`
  minus its `processFileCalled` recorder — the one probe that catches the
  `processFile` bug above. Merged, and the declined-file test now asserts it.

- Two locals named `uploader` shadowed the new `MediaUploader` property, silently
  (kotlinc has no diagnostic for it, detekt no rule). Renamed to `client`.
- `RecordingUploader`'s fixture carried no `title`, so the repo's only worked
  example of an uploader result was a body that trips `transformAttachment`.
* feat!: remove MediaUploadDelegate.uploadFile

`MediaUploader` replaces it. Returning a raw response split one upload's
HTTP across two owners — the host performed the `POST`, the editor drove
the `post-process` retries and orphan cleanup behind it — and the hook
received no form fields, so an attachment it uploaded landed unattached to
its post. Neither is fixable while the hook returns a raw response, which
is what the replacement changes.

What is left is a clean division: a delegate transforms bytes and
GutenbergKit owns delivery and its retries; a `MediaUploader` owns
delivery and its retries entirely. There is no longer an in-between where
the host performs the upload but the editor retries it.

`handlesFile` no longer gates the temp copy for two callers, only for
`processFile` — and only when no uploader is set, since an uploader takes
over delivery for every file.

`MediaUploadResponse` drops to internal on both platforms: `uploadFile`
was the only public API that named it.

BREAKING CHANGE: hosts implementing `uploadFile` must conform to
`MediaUploader` instead. Hosts that only implement `processFile` /
`handlesFile` are unaffected.

* docs: correct the handlesFile contract and the delegate's stale upload docs

Review follow-ups to 6bcc210. No behavior change.

`handlesFile`'s new doc said it is "only consulted when no `MediaUploader` is
set". It is always consulted (`MediaUploadServer.swift:143`, `.kt:374`), and it
still gates `processFile` (`.swift:306`, `.kt:534`) — a declined file reaches
the uploader unprocessed. The implementation comment 160 lines away and the
`an uploader sees a file the delegate's metadata gate would have declined` test
on both platforms already said so. Replaced with wording lifted from that
comment.

Removing `uploadFile` also left the docs a host actually reads still
advertising it:

- The `mediaUploadDelegate` property summaries — what Xcode Quick Help and IDE
  hover show — said "customizing media file processing and upload behavior"
  (iOS) and "(resize, transcode, custom upload)" (Android). Both now describe
  transformation and point at `mediaUploader` for the upload case.
- `MediaUploadResponse.statusCode` claimed the status could come from "the
  host's upload service". `MediaUploader.upload` returns `Data`, so the host
  path supplies a literal 201.
- `MediaUploadServer`'s parameter docs, the `UploadResult.uploaded` doc, and
  Android's "won't process or upload" comment, whose iOS twin already read
  "won't process".

Two non-doc changes ride along:

- `UploadError.noUploader`'s message named a role the delegate no longer has:
  "No upload delegate or internal media client configured" becomes "No media
  uploader or ...". It reaches the editor in a 500 body; nothing asserts on it.
- iOS's `MockUploadDelegate` became a duplicate of `ProcessOnlyDelegate` once
  `uploadFile` went. Android already consolidated on `ProcessOnlyDelegate`;
  iOS now matches.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… class bound (#685)

* refactor!: rename MediaUploadDelegate to MediaProcessor

The protocol no longer uploads anything — the previous commit removed
`uploadFile`, leaving `handlesFile` and `processFile`. "UploadDelegate" now
describes the one thing it can't do, and next to `MediaUploader` the two
names read as variations on the same job rather than the two halves of a
deliberate split.

`MediaProcessor` says what is left: it transforms bytes, GutenbergKit
delivers them. Mechanical throughout — the property becomes
`mediaProcessor`, the server parameter `processor`, the file
`MediaHandlers.swift` (it holds both protocols now), and Android's demo
`DemoMediaProcessor`. Prose follows the types.

The `weak_delegate` suppression added when the property became strong goes
away with the name: the rule was arguably right that a strongly-held
"delegate" is a smell, and the answer was that this was never a delegate.

BREAKING CHANGE: `mediaUploadDelegate` is now `mediaProcessor`, and
`MediaUploadDelegate` is `MediaProcessor`. Conformances need no changes
beyond the name.

* refactor(ios)!: drop the class requirement from the media protocols

`MediaProcessor` and `MediaUploader` were both `AnyObject`-bound, and
`EditorViewController` holds both strongly. A conformer that holds the view
controller back therefore closes a retain cycle ARC cannot break: the editor
is never freed, so `deinit` never runs, so `uploadServer.stop()` — its only
caller — never runs either, and a bound loopback `NWListener` outlives the
editing session.

Nothing needed class-boundness. There is no `weak`, `===`, or
`ObjectIdentifier` use against either protocol anywhere in the tree, and
every existing conformer is a class, which conforms unchanged. Dropping the
requirement lets a host conform with a value type capturing only what the
work needs — the shape that avoids the cycle, and the one a class-bound
`Delegate` discouraged.

This does not make the cycle impossible: a struct that stores the view
controller cycles just the same. The docs say so rather than implying the
type system settles it.

* fix(ios): name every media handler in the leak census, not just the first

`countServerStarted` resolved its name as `processor.map { … } ?? uploader.map { … }`,
so with both supplied it always named the processor. The retainer is as likely to be
the uploader — and after the class bound came off `MediaProcessor`, the processor it
names may be a value type holding nothing at all, which is the one shape that provably
cannot close the cycle the fault is reporting.

A host following the docs hits this on the recommended shape: a leaf processor for the
transform plus an uploader on the coordinator that owns the editor. The fault named the
leaf, so the reader audits an object with no stored references, finds nothing, and
concludes the census is broken.

Names every handler that was supplied, and softens the assertion from "is its own media
handler" to "is one of its own media handlers" — with two names it is a candidate list,
not an accusation.

DEBUG-only, and still behind the `count >= liveServerLeakThreshold` guard, so
`String(describing: type(of:))` stays off the start path.

* fix(ios): start the upload server for a host that supplies only an uploader

`startUploadServer()` asks "did the host supply a media handler" twice — once before
starting, once after the bind returns, because `stopMediaHandling()` can land while
that `await` is suspended. The two reads had drifted. #628 widened the first to
`delegate != nil || uploader != nil` and left the second checking the delegate alone.

So a host that passed only a `mediaUploader` cleared the entry check, bound a loopback
listener, then failed the post-bind check and stopped the server it had just started.
`uploadServer` stayed nil, `buildEditorConfiguration` advertised `nativeUploadPort:
nil`, and `api-fetch.js` fell through to the plain WebView path. The host's `upload(_:)`
was **never called, for any file** — no error, no log. Uploads appeared to work; they
just never reached the host's background session, offline queue, or retry policy,
which is the whole reason to supply an uploader.

Both reads now go through one `hasMediaHandling`, so they cannot disagree again. That
is the actual defect — two hand-maintained copies of one predicate — and it is the same
failure `MediaServerCredentials` was extracted for, where a check "diverged silently
between iOS and Android once".

`uploadServer` and `startUploadServer()` become internal so the suite can reach them.
The test is parameterized over uploader-only, processor-only and both. Mutation-checked:
restoring the old post-bind guard fails **only** the uploader-only case, which is the
regression and nothing else. Android already pinned this gate
(`GutenbergViewUploadServerTest`, "the upload server starts for an uploader with no
delegate"); iOS had no equivalent, which is why the drift survived three commits green.

* test: finish the rename in the media suites' own vocabulary

The rename swept the helper types and left the names around them. 101 sites across four
files: iOS test functions and `@Test` display strings, Kotlin backtick names, `let
delegate = ProcessOnlyProcessor()` bindings that contradicted themselves on one line,
`weakDelegate`, and a `// MARK: - Upload with delegate` header over code the production
file had already renamed to `// MARK: - Processor Pipeline`.

These are the strings CI prints. A red build named `retainsDelegateForServerLifetime` or
`processes with the delegate, then delivers through the internal client` for a codebase
where no symbol contains the word — Kotlin backticks are literally the JUnit report
strings — so the first move on a failure was to grep for an API this stack deleted.

Safe as a plain substring replacement: none of the four files reference a genuine
delegate. `HttpServerDelegate` and `EditorViewControllerDelegate` live in other test
files and are untouched.

Two names would have read as stutters after a mechanical pass, so they say what the test
does instead: `processesThenDelivers` and `processorRunsForUploader`.

Test counts are unchanged — 590 iOS, and Android green on `--rerun-tasks` — so this
renames tests rather than adding or dropping any. Note it does reset Buildkite Test
Analytics history for the renamed cases, which is the deliberate cost.

* test(ios): pin that a value type can conform to MediaProcessor

Dropping `: AnyObject` is what the second commit here exists to deliver, and nothing
exercised it — all eleven conformers in the tree were classes, so the boxed-existential
path was never walked: copied into `UploadContext`, captured by the `@Sendable` handler
closure, read again at `processFile`.

Re-imposing the class bound, or breaking that path, would have compiled and passed
green and surfaced only in a host's build. It now fails at compile time:

    error: non-class type 'ValueTypeProcessor' cannot conform to class protocol 'MediaProcessor'

`ValueTypeProcessor` is `Sendable` without `@unchecked` — also the point, since that is
the shape the protocol's documentation now recommends and the escape hatch it describes.

The assertions run through `MockInternalMediaClient`'s recorded metadata rather than
state on the processor, because a `struct` witnessing a non-mutating requirement cannot
record anything. The transcoded mimeType and filename reaching the client could only
come from `processFile` having actually run, so this pins invocation, not just storage.
…686)

`MediaUploadServer` handled requests through static functions threading an
`UploadContext` parameter through every call, because the closure form of
`HTTPServer.start` can't capture the object that owns the server: the
closure has to exist before the server does, and capturing `self` would
form `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer`, so
`deinit` — and its `stop()` — would never run.

Add an `HTTPRequestHandler` protocol to `GutenbergKitHTTP` and a `start`
overload that takes one, then serve `MediaUploadServer` from it. The
dependencies become stored properties on a `Handler` struct and the request
logic becomes instance methods. The closure overload is unchanged and
forwards to the same code path, so the addition is purely additive — no
existing caller, test, or the debug server is affected. Request handling is
mandatory, so it can't be a defaulted `HTTPServerDelegate` method the way
optional customization points are; hence an overload rather than a new
delegate requirement.

The protocol is deliberately not `AnyObject`-constrained so a handler *can*
be a struct holding only what it needs — not because a struct is safe by
construction. A value type is not protection: the server captures the
handler into a heap node, so a struct storing the server's owner closes the
same ring a class would. Both shapes work, under the same leaf discipline
`HTTPServerDelegate` already documents — a handler must not strongly hold
the object that owns the server. `Handler` stores no reference back to the
`MediaUploadServer`, which is why the helpers outside it stay static.

Mechanically: `handleRequest` becomes `handle`, the functions that use the
dependencies become instance methods, and the ones that don't
(`attachmentId`, `relayResponse`, `uploadErrorResponse`, `formFields`) stay
static. `UploadContext` goes away — `Handler` is what it was. Helpers
outside the handler (`errorResponse`, `writeStream`, `sanitizeFilename`,
`uploadsTempDirectory`) are qualified rather than moved.

No behavior change.
Setting a `mediaUploader` means the host is taking over uploads. With no
site credentials the server would previously just not start, silently
dropping the uploader — and its media deletes still need the internal
media client to reach the configured site, since every attachment lives
there no matter who delivered it.

So the behavior forks by intent. A `mediaProcessor` with no credentials
leaves the server down and uploads fall to the default WebView path —
there is nothing to deliver through, so nothing to process. A
`mediaUploader` with no credentials is a configuration error and fails
fast: `precondition` on iOS, `check` on Android.

The check runs where the host states its intent, not at page load. iOS
takes its handlers at `init` and holds them `private(set)`, so a non-nil
uploader at load time was necessarily passed at construction — checking
there puts the caller's own line in the stack trace instead of surfacing
the mistake from inside a page-load callback that names only GutenbergKit.
Android still takes its handlers as mutable properties, so the earliest
equivalent point is the `mediaUploader` setter, beside the existing
set-before-load `check`. This is the shape `359d89ad` already established
for the set-before-load contract: enforce the rule where the host states
its intent.

Android gains a `MediaServerCredentials` of its own, mirroring iOS's.
The two predicates had diverged — iOS required an absolute site root while
Android checked only `isEmpty()` — so `siteApiRoot = "example.com/wp-json/"`,
which is what a user types when asked for their site address, trapped on
iOS and started a doomed server on Android. Every relayed delete then threw
`IllegalArgumentException` out of OkHttp's `.url()`, which is not an
`IOException`, so it escaped `handleDelete` and degraded to a plain-text
500 the editor cannot parse — orphan cleanup failing silently. Both sides
now test scheme and host. Emptiness is tested alongside nullity because
`Uri` and `URL` disagree on a missing authority: `file:///tmp/wp-json`
yields a null host on iOS and an empty one on Android.

The policies live outside the view types on both platforms so they are
reachable from the host test suites. On iOS that is load-bearing:
`EditorViewController` is `#if canImport(UIKit)` and therefore absent from
the macOS host, the one platform that can run Swift Testing's exit tests,
so the trap itself is testable rather than only the predicate. The two
suites assert matching cases on purpose — this policy has diverged silently
once, and matching cases make the next divergence a failing test rather
than a crash on one platform and a broken server on the other.

A `mediaUploader` can still be dropped without failing, at the cleartext
guard: an app that has not permitted cleartext to localhost never reaches
the loopback server, so the uploader is never called. That one logs and
degrades rather than failing, and the distinction is the cause rather than
the symptom — missing credentials is an incoherent configuration, while
blocked cleartext is a sound configuration the app's network policy blocks,
and permitting cleartext makes the same setup work unchanged. Nothing had
told integrators to permit it, so `docs/integration.md` now does, including
why the library cannot ship the config itself: `networkSecurityConfig` is a
single-valued `<application>` attribute, so a library declaring it fails the
manifest merge against the host's and against other libraries that declare
one — `rs.wordpress.api` already does.
`formFields` decodes every non-file form part as UTF-8. That can only be
lossless because of who is on the other end, and nothing in the code enforces
it -- so write it down on both platforms, in plain terms: only the editor's own
page can reach the server, the browser guarantees form-field text is valid
Unicode, and raw bytes always arrive carrying a filename, which routes them to
the file rather than to a field.

Pin the third condition with tests, since it is the one this code could break on
its own. The bodies are ordered the way `uploadToServer` actually emits them --
file first, then additionalData -- and assert both the uploaded filename and the
decoded fields, so the suite fails if either the partition or the file-selection
rule changes. A second test covers the other half: valid UTF-8 round-trips, so
real captions and titles are unaffected.

The `buildMultipart` comments also claimed raw bytes were appended so a non-UTF-8
value would be "forwarded verbatim rather than coerced". That is not why -- such
a value cannot reach them. On iOS they avoid a failable `String(data:encoding:)`
whose `?? ""` would quietly drop a whole field; on both platforms they keep the
re-encode byte-for-byte identical to the passthrough it replaces.
…ck (#689)

`TranscodingProcessor` duplicated `ResizingProcessor` — same
`.processed(_, mimeType: "video/mp4", filename: "clip.mp4")` result, one call
site — and was the weaker of the two. It wrote to a fixed `$TMPDIR/clip.mp4`
instead of a per-call UUID path inside the managed upload directory, and
swallowed the write with `try?`, so a failed write still returned
`.processed(<nonexistent URL>, …)` and the test passed green against a file
that never existed. `ResizingProcessor` uses `try` and a unique path.

Also drops `@unchecked Sendable` from `ThrowingUploader`, which has no stored
properties and so satisfies `MediaUploader`'s inherited `Sendable` conformance
on its own. The escape hatch is only needed by the mocks holding
`NSLock`-guarded state; carrying it on a stateless one normalizes it as
boilerplate, which is how an unsynchronized property gets added later without
a diagnostic. `ContentTypeDeleteClient` keeps it — it subclasses
`InternalMediaClient`, itself an `@unchecked Sendable` class, and must restate
the conformance.
#651)

`viewDidDisappear` cancelled `dependencyTaskHandle`, the async editor
dependency fetch. That callback fires whenever the editor is merely
covered — a full-screen modal presented over it, a push on top of it, a
tab switch — and the fetch has exactly one starting point, the "no
dependencies" branch of `viewDidLoad`, with nothing that restarts it.
Cover a still-loading editor that way and the load is over for good:
with the fetch parked mid-flight and `viewDidDisappear` delivered, the
simulator shows the progress view replaced by the load-error screen and
the host told `didFailToLoad` with a cancellation error. Coming back to
the editor does nothing.

The fast path a few lines above already carried the fix for this class
of failure — the same cancellation landing mid `startUploadServer()`
silently disabled native uploads for the session (#357) — but the async
path never got the same treatment. Its task ends in the same
`loadEditor()`, so that reason covers it too; its comment now says so,
along with its own: nothing restarts the fetch.

Stop cancelling rather than cancel-and-restart. A restart path would have
to be idempotent and not race a fetch already in flight — complexity with
nothing to buy.

`deinit` is not an alternative home for the cancellation either, which is
why `dependencyTaskHandle` goes away with the override rather than moving
there. The task body is `await self?.prepareEditor()`, and optional-
chaining a weak `self` into an async call holds a *strong* `self` across
every suspension inside it, so the editor cannot be deallocated while the
fetch is running. `deinit` is reachable only once the task has already
finished, where there is nothing left to cancel.

Not cancelling has a cost. The same retain keeps an editor released
mid-fetch alive until the fetch and the load after it finish, which only
URL timeouts bound. Meanwhile it keeps writing to the site's caches, and
once the fetch lands it binds its upload server: a host that retains its
own editor strands one more listener, and the DEBUG leak census can fire
on a slow network. `[weak self]` still makes a task that has not started
yet a no-op on an editor released first.

Gating the cancellation on `isBeingDismissed`/`isMovingFromParent` was not
an option. Hosts install this controller as a child, so UIKit sets those
flags on an ancestor and they read `false` here — the gate would never
fire, which is this change with a misleading condition on top.

`EditorViewControllerLifecycleTests` pins both halves: covering the editor
leaves the fetch running, and the fetch holds the editor alive until it
finishes and releases it then. Against the old code the first fails with
the real symptom, a cancelled request. The tests inject a
`URLSessionProtocol` that holds every request until released, so the
editor runs its real fetch path, and cover the editor through
`beginAppearanceTransition`/`endAppearanceTransition` — `begin` alone
never delivers `viewDidDisappear`. Each uses a fresh site host and deletes
what it wrote, since `EditorViewController` can't be pointed at a
temporary directory.
@jkmassel
jkmassel force-pushed the fix/own-media-delegate-strongly branch from b8d49ad to 23451f0 Compare October 2, 2026 02:07
…iddleware' into fix/own-media-delegate-strongly
…g stops

`revokeNativeUploadEndpoint()` cleared the endpoint from three places: the
live page, the injected user script, and the `localStorage` copy of `GBKit`
that `getGBKit()` fell back to. #613 removed that copy — `getGBKit()` reads
`window.GBKit` alone, and the document-start script now removes the key so
nothing session-scoped persists across launches.

Merged together, the revoke no longer cleared anything there. It created
the key instead: `JSON.parse(localStorage.getItem('GBKit') || '{}')` found
nothing, so it stored `{"nativeUploadPort":null,"nativeUploadToken":null}`.
No credential, and the next document start removed it again, but it is a
write to storage #613 exists to keep empty, for a reader that is gone.

Drop the block. Two copies hold the endpoint now, and the doc says so.
The last ten lines of the protocol's doc comment restated the two
paragraphs above them — the `Sendable` requirement on a captured reference
and the two `struct` mistakes that compile silently — starting mid-sentence
after "a processor that is never called". Xcode Quick Help rendered both
copies.
@jkmassel jkmassel changed the title fix(ios)!: own the media upload delegate, and give hosts a way out feat!: replace MediaUploadDelegate with an editor-owned MediaProcessor and MediaUploader Oct 2, 2026
…701) (#752)

* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

iOS [Type] Breaking Change For PRs that introduce a change that will break existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants