fix(ios): own the media upload delegate instead of holding it weakly

ios/Sources/GutenbergKit/Sources/EditorViewController.swift

@@ -109,28 +109,24 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro
     /// take effect, so its setter traps if written.
     private var hasStartedLoading = false
 
-    /// Whether a non-nil ``mediaUploadDelegate`` was ever assigned. Lets the load
-    /// path tell "the delegate was released before load" (a retention mistake to
-    /// trap) apart from "no delegate was configured" (a valid opt-out).
-    private var mediaUploadDelegateWasAssigned = false
-
     /// Delegate for customizing media file processing and upload behavior.
     ///
     /// Provide this **before the editor loads** — typically right after `init`, the
     /// same way the rest of the editor configuration is supplied. It is captured
     /// once, when the editor begins loading, and injected into the page's initial
     /// configuration; setting it afterward has no effect, so the setter traps.
     ///
-    /// - Important: This is a `weak` reference — you must hold a strong reference to
-    ///   your delegate until the editor has loaded, or native uploads are silently
-    ///   disabled. To surface that mistake, the editor traps at load time if a
-    ///   delegate that was assigned here has already been deallocated.
-    public weak var mediaUploadDelegate: (any MediaUploadDelegate)? {
+    /// The editor **owns** this for its lifetime and releases it on `deinit`, so you
+    /// don't need to keep a reference after assigning it. The one rule: your delegate
+    /// must not strongly retain this `EditorViewController` in return, or the two form
+    /// a retain cycle and neither is freed.
+    // Ownership here is the point: the editor holds this for its lifetime so an
+    // in-flight upload can't lose the delegate mid-request. The cycle `weak_delegate`
+    // guards against runs the other way (a delegate retaining the editor), which this
+    // property can neither create nor prevent.
+    // swiftlint:disable:next weak_delegate
+    public var mediaUploadDelegate: (any MediaUploadDelegate)? {
         didSet {
-            // Record whether a delegate was provided so the load path can tell a
-            // premature deallocation apart from a deliberate opt-out (see
-            // `startUploadServer`).
-            mediaUploadDelegateWasAssigned = mediaUploadDelegate != nil
             // Deliberate fail-fast, not a defensive check. The delegate is captured
             // into the page's initial configuration when the editor begins loading,
             // so a delegate assigned afterward would silently never take effect;
@@ -451,14 +447,10 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro
     /// falls back to Gutenberg's default upload behavior (the JS override won't activate
     /// because `nativeUploadPort` will be nil in GBKit).
     private func startUploadServer() async {
-        // A delegate that was provided but is already nil here was deallocated before
-        // the editor finished loading — the host didn't hold a strong reference to it.
-        // That silently disables native uploads, so trap loudly instead.
-        precondition(
-            !(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil),
-            "mediaUploadDelegate was released before the editor loaded — hold a strong reference to it."
-        )
-
+        // Nothing to route through the native server unless the host provided a
+        // delegate. The editor owns it — `mediaUploadDelegate` is strong — so there's
+        // no released-before-load case to guard against; it lives as long as the
+        // editor does.
         guard mediaUploadDelegate != nil else {
             return
         }

ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift

@@ -453,25 +453,24 @@ enum UploadError: Error, LocalizedError {
 // MARK: - Upload Context
 
 /// Container for the upload delegate and default uploader, captured by the
-/// HTTPServer handler closure and re-read on each request.
+/// HTTPServer handler closure and read on each request.
 ///
-/// The delegate is held **weakly**. `EditorViewController.mediaUploadDelegate` is
-/// declared `weak` — the host owns the delegate's lifetime. Capturing it strongly
-/// here would silently defeat that contract and, worse, risk a retain cycle
-/// (`EditorViewController → uploadServer → HTTPServer → handler → UploadContext →
-/// delegate → EditorViewController`) that would keep the view controller — and
-/// therefore the server — alive forever, so `deinit` would never stop it.
+/// Both are held **strongly**, so a delegate that admitted a file for processing
+/// will process it — the three reads within a request can't disagree, and an
+/// in-flight upload keeps the host's delegate alive until it unwinds. This matches
+/// Android, which holds its `uploadDelegate` as a plain `val` for the same reason.
 ///
-/// `@unchecked Sendable`: `uploadDelegate` is assigned once at init and only read
-/// afterwards; weak-reference reads are thread-safe at runtime.
-private final class UploadContext: @unchecked Sendable {
-    weak var uploadDelegate: (any MediaUploadDelegate)?
+/// Strong is safe because `EditorViewController` owns `mediaUploadDelegate` strongly
+/// too. A host object that retains the view controller back already forms
+/// `EditorViewController → mediaUploadDelegate → EditorViewController`, a cycle this
+/// container can neither create nor prevent — so holding weak here bought no leak
+/// protection, only the risk of the delegate vanishing mid-request.
+///
+/// A `struct`, so it is implicitly `Sendable`: `MediaUploadDelegate` is a `Sendable`
+/// protocol and `DefaultMediaUploader` is `@unchecked Sendable`.
+private struct UploadContext: Sendable {
+    let uploadDelegate: (any MediaUploadDelegate)?
     let defaultUploader: DefaultMediaUploader?
-
-    init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) {
-        self.uploadDelegate = uploadDelegate
-        self.defaultUploader = defaultUploader
-    }
 }
 
 // MARK: - Default Media Uploader

ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift

@@ -385,24 +385,75 @@ struct MediaUploadServerTests {
     #expect(FileManager.default.fileExists(atPath: fresh.path(percentEncoded: false)))
   }
 
-  @Test("does not strongly retain the upload delegate (weak — preserves deinit teardown)")
-  func doesNotStronglyRetainDelegate() async throws {
+  @Test("retains the delegate for the server's lifetime, and releases it after")
+  func retainsDelegateForServerLifetime() async throws {
     weak var weakDelegate: MockUploadDelegate?
-    let server: MediaUploadServer
+    var server: MediaUploadServer?
     do {
       let delegate = MockUploadDelegate()
       weakDelegate = delegate
       server = try await MediaUploadServer.start(uploadDelegate: delegate)
     }
-    defer { server.stop() }
 
-    // UploadContext holds the delegate weakly, so releasing the host's strong
-    // reference deallocates it. A strong reference here would reintroduce the
-    // EditorViewController → uploadServer → … → delegate → EditorViewController
-    // cycle, so deinit would never fire and the server would never stop.
+    // The server owns the delegate while it runs: the host can assign one and drop
+    // its own reference, and every request still sees it.
+    #expect(weakDelegate != nil)
+
+    server?.stop()
+    server = nil
+
+    // …and lets go when it does, so the delegate isn't leaked for the process's
+    // lifetime. The cycle the old `weak` was defending against runs through
+    // `EditorViewController.mediaUploadDelegate`, which this container can neither
+    // create nor prevent.
+    //
+    // Polled rather than asserted outright: the handler closure is captured by the
+    // listener's `newConnectionHandler`, and `NWListener.cancel()` is asynchronous —
+    // the framework holds the listener until cancellation completes, so the release
+    // trails `stop()` by a beat. A leak still fails this, just after a second.
+    for _ in 0..<100 where weakDelegate != nil {
+      try await Task.sleep(for: .milliseconds(10))
+    }
     #expect(weakDelegate == nil)
   }
 
+  @Test("still processes for a delegate the host has dropped its reference to")
+  func processesForHostReleasedDelegate() async throws {
+    // The delegate is read at the admission gate and again at processFile and
+    // uploadFile, separated by a synchronous disk copy and an unbounded processFile.
+    // Held weakly, a host that dropped its reference changed the answer between
+    // those reads: a file admitted for processing was forwarded unprocessed. The
+    // host dropping it before the request is the same condition, deterministically.
+    let mockUploader = MockDefaultUploader()
+    var delegate: TranscodingDelegate? = TranscodingDelegate()
+    weak var weakDelegate = delegate
+    let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader)
+    defer { server.stop() }
+
+    // Drop the host's only strong reference. Under the documented contract the
+    // server owns the delegate from here, so the upload must still be processed.
+    delegate = nil
+
+    let boundary = UUID().uuidString
+    let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8))
+    let url = URL(string: "http://127.0.0.1:\(server.port)/upload")!
+    var request = URLRequest(url: url)
+    request.httpMethod = "POST"
+    request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization")
+    request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type")
+    request.httpBody = body
+
+    _ = try await URLSession.shared.data(for: request)
+
+    // The server kept it alive, so the processed metadata reached the uploader.
+    // Against a weak container this fails with the real symptom: the passthrough
+    // branch runs and the original video/quicktime is forwarded unprocessed.
+    #expect(weakDelegate != nil)
+    #expect(mockUploader.uploadCalled)
+    #expect(mockUploader.lastUploadMimeType == "video/mp4")
+    #expect(!mockUploader.passthroughUploadCalled)
+  }
+
   private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data {
     var body = Data()
     body.append("--\(boundary)\r\n")
@@ -791,6 +842,20 @@ private func readAllFromStream(_ stream: InputStream) -> Data {
 
 // MARK: - Mocks
 
+/// A delegate that transcodes, used to check the server holds it across the whole
+/// request rather than re-reading a reference the host may have dropped.
+private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendable {
+  func handlesFile(ofType mimeType: String, named filename: String) -> Bool {
+    true
+  }
+
+  func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile {
+    let processed = FileManager.default.temporaryDirectory.appendingPathComponent("clip.mp4")
+    try? Data("transcoded".utf8).write(to: processed)
+    return .processed(processed, mimeType: "video/mp4", filename: "clip.mp4")
+  }
+}
+
 private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable {
   private let lock = NSLock()
   private var _processFileCalled = false