Move synchronous image upload preparation off the main actor (#6323)

* Move synchronous image upload preparation off the main actor

Signed-off-by: Ivan Magda <imagda15@gmail.com>

* Make media preparation cooperative with cancellation

Replace the dispatch queue with concurrent copy, image and video-thumbnail
functions. Return sendable upload metadata directly, bound each batch to
two preparations and discard unfinished results when cancellation occurs.

Keep file metadata reads synchronous and prevent the preview from sending
a result after its preparation task has been cancelled. Replace unrelated
tests with cancellation and batch-order coverage requested during review.

* Update MediaUploadingPreprocessor.swift

* Run media preparation in a nonisolated preprocessor

Make MediaUploadingPreprocessor a nonisolated struct whose copy, image
processing and video thumbnail run in @concurrent instance methods,
instead of a main-actor type with static helpers and an extra setting
parameter.

Check cancellation with plain early returns between the slow steps,
name the batch limit, stop logging cancelled video exports as errors,
and restore develop's structure and doc comments where they were changed
without need.

Replace the tests that didn't reach the new code with tests for cleanup
on failure, on batch failure and on cancellation, and make the preview
test set up its blocking mock before the view model starts processing.

* Apply review suggestions to media preparation

Docs:
- Drop the doc comment on maximumConcurrentProcessingCount, the name says it.
- Move the cleanup note into the main description of both processMedia
  functions, so the Returns lines match develop again.

Tests:
- Rename the test helper to copyResourceToTemporaryDirectory(_:).

---------

Signed-off-by: Ivan Magda <imagda15@gmail.com>
Co-authored-by: Ivan Magda <imagda15@gmail.com>
This commit is contained in:
Mauro
2026-10-13 17:33:22 +02:00
committed by GitHub
parent e1780f322a
commit b31950789f
5 changed files with 237 additions and 55 deletions
@@ -34,6 +34,35 @@ final class MediaUploadPreviewScreenViewModelTests {
userIndicatorController = UserIndicatorControllerMock()
}
@Test
func cancelWhileSendWaitsForPreparation() async throws {
// Given a screen whose media preparation is still running.
let (startedStream, startedContinuation) = AsyncStream<Void>.makeStream()
let (resumeStream, resumeContinuation) = AsyncStream<Void>.makeStream()
defer { resumeContinuation.finish() }
let preparationStarted = deferFulfillment(startedStream) { _ in true }
setUpViewModel(urls: [imageURL], expectedCaption: nil) {
startedContinuation.yield()
var iterator = resumeStream.makeAsyncIterator()
_ = await iterator.next() // Only returns once the preparation is cancelled.
return .success(100 * 1024 * 1024)
}
try await preparationStarted.fulfill()
// When sending and then dismissing the screen before the preparation finishes.
context.send(viewAction: .send)
let loadingFinished = deferFulfillment(context.observe(\.viewState.shouldDisableInteraction)) { !$0 }
context.send(viewAction: .cancel)
viewModel.stopProcessing() // Called by the coordinator when the screen is dismissed.
try await loadingFinished.fulfill()
// Then nothing is sent and no error is shown.
#expect(timelineProxy.sendImageUrlThumbnailURLImageInfoCaptionRequestHandleCallsCount == 0)
#expect(userIndicatorController.submitIndicatorDelayCallsCount == 1, "Only the loading indicator should be shown")
#expect(context.viewState.bindings.alertInfo == nil)
}
@Test
func imageUploadWithoutCaption() async throws {
setUpViewModel(urls: [imageURL], expectedCaption: nil)
@@ -293,7 +322,8 @@ final class MediaUploadPreviewScreenViewModelTests {
expectedCaption: String?,
maxUploadSizeResult: Result<UInt, ClientProxyError>? = nil,
simulateGallerySendFailure: Bool = false,
galleryEnabled: Bool = true) {
galleryEnabled: Bool = true,
maxUploadSizeClosure: (() async -> Result<UInt, ClientProxyError>)? = nil) {
timelineProxy = TimelineProxyMock(.init())
timelineProxy.sendAudioUrlAudioInfoCaptionRequestHandleClosure = { [weak self] _, _, caption, _ in
self?.verifyCaption(caption, expectedCaption: expectedCaption) ?? .failure(.sdkError(TestError.unknown))
@@ -316,6 +346,7 @@ final class MediaUploadPreviewScreenViewModelTests {
if let maxUploadSizeResult {
clientProxy.underlyingMaxMediaUploadSize = maxUploadSizeResult
}
clientProxy.maxMediaUploadSizeClosure = maxUploadSizeClosure
viewModel = MediaUploadPreviewScreenViewModel(mediaURLs: urls,
caption: nil,
@@ -435,8 +435,90 @@ final class MediaUploadingPreprocessorTests {
#expect(optimizedImageInfo.height == 2048)
}
@Test
func batchPreservesInputOrderBeyondConcurrencyLimit() async throws {
let names = ["test_image.png", "test_animated_image.gif", "landscape_test_image.jpg"]
try #require(names.count > MediaUploadingPreprocessor.Constants.maximumConcurrentProcessingCount)
let inputs = try names.map { try #require(Bundle(for: Self.self).url(forResource: $0, withExtension: nil)) }
let results = try await mediaUploadingPreprocessor.processMedia(at: inputs, maxUploadSize: maxUploadSize).get()
defer {
for result in results {
try? FileManager.default.removeItem(at: result.url.deletingLastPathComponent())
}
}
#expect(results.map(\.url.lastPathComponent) == names)
for result in results {
let thumbnailURL = try #require(result.thumbnailURL)
#expect(try Data(contentsOf: result.url).isEmpty == false)
#expect(try Data(contentsOf: thumbnailURL).isEmpty == false)
}
}
@Test
func failedPreparationRemovesItsFiles() async throws {
let url = try copyResourceToTemporaryDirectory("test_image.png")
defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) }
guard case .failure(.maxUploadSizeExceeded) = await mediaUploadingPreprocessor.processMedia(at: url, maxUploadSize: 1) else {
Issue.record("Expected the upload size to be exceeded")
return
}
#expect(try preparedFolders(for: url).isEmpty)
}
@Test
func batchFailureReturnsTheErrorAndRemovesPreparedFiles() async throws {
let url = try copyResourceToTemporaryDirectory("test_image.png")
defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) }
let missingURL = url.deletingLastPathComponent().appendingPathComponent("missing.png")
// The missing file comes last so that at least one image has already been prepared when the batch fails.
guard case .failure(.failedProcessingMedia) = await mediaUploadingPreprocessor.processMedia(at: [url, url, missingURL], maxUploadSize: maxUploadSize) else {
Issue.record("Expected the missing file to fail the batch")
return
}
#expect(try preparedFolders(for: url).isEmpty)
}
@Test
func cancellationRemovesPreparedFiles() async throws {
let url = try copyResourceToTemporaryDirectory("test_image.png")
defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) }
let task = Task { await mediaUploadingPreprocessor.processMedia(at: url, maxUploadSize: maxUploadSize) }
await Task.yield() // Let the preparation start.
task.cancel()
guard case .failure(.cancelled) = await task.value else {
Issue.record("Expected the preparation to be cancelled")
return
}
#expect(try preparedFolders(for: url).isEmpty)
}
// MARK: - Private
/// Copies a test asset to a uniquely named file, so the folders the preprocessor prepares from it can be found afterwards.
private func copyResourceToTemporaryDirectory(_ resource: String) throws -> URL {
let assetURL = try #require(Bundle(for: Self.self).url(forResource: resource, withExtension: nil))
let directory = FileManager.default.temporaryDirectory.appendingPathComponent("\(Self.self)-\(UUID().uuidString)")
try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true)
let url = directory.appendingPathComponent("\(UUID().uuidString).\(assetURL.pathExtension)")
try FileManager.default.copyItem(at: assetURL, to: url)
return url
}
/// The unique folders created by the preprocessor for the given file that still exist.
private func preparedFolders(for url: URL) throws -> [URL] {
let name = url.deletingPathExtension().lastPathComponent
return try FileManager.default.contentsOfDirectory(at: FileManager.default.temporaryDirectory, includingPropertiesForKeys: nil)
.filter { UUID(uuidString: $0.lastPathComponent) != nil }
.filter { folder in
let contents = (try? FileManager.default.contentsOfDirectory(atPath: folder.path(percentEncoded: false))) ?? []
return contents.contains { $0.contains(name) }
}
}
private func isEqual<N: UnsignedInteger>(_ lhs: N, _ rhs: N, within tolerance: N) -> Bool {
isEqual(Double(lhs), Double(rhs), within: Double(tolerance))
}