From 04fd1bfd0e6bb358c4e858e077d30bfc3ca3aba2 Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Mon, 15 Jun 2026 12:49:01 +0300 Subject: [PATCH] Serialise client pausing and resuming to fix the foreground resume race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pause and resume previously ran as independent, unsynchronised tasks: `pauseServices` stopped the sync and paused the client from a detached `Task`, while `resumeServices` resumed and started the sync directly. With no ordering between them, a late background pause could land after a foreground resume — leaving the client suspended and the sync stopped while the app was active, which broke everything until the next relaunch. Route both through a single serialised reconciler: each request records the desired state and chains onto the previous transition, and the reconcile always drives the SDK to the *latest* desired state. Transitions can no longer interleave and the most recent intent always wins. On deinit we no longer pause the client — it's being deallocated, so the SDK tears it down anyway; we just stop the sync and cancel the delegate. --- .../Sources/Services/Client/ClientProxy.swift | 109 ++++++++++++------ 1 file changed, 72 insertions(+), 37 deletions(-) diff --git a/ElementX/Sources/Services/Client/ClientProxy.swift b/ElementX/Sources/Services/Client/ClientProxy.swift index 7a8e29f32..97232f49a 100644 --- a/ElementX/Sources/Services/Client/ClientProxy.swift +++ b/ElementX/Sources/Services/Client/ClientProxy.swift @@ -154,7 +154,12 @@ class ClientProxy: ClientProxyProtocol { private var hasEncounteredAuthError = false deinit { - pauseServices { [delegateHandle] in + // Stop the sync and cancel the delegate (in that order) before the client is released. No need to pause + // the client as it's being deallocated, so the SDK tears it down anyway. Capture the sync service strongly + // as `self` is being deallocated. + Task { [syncService, delegateHandle] in + await syncService.stop() + // The delegate handle needs to be cancelled always after the sync stops delegateHandle?.cancel() } @@ -402,21 +407,7 @@ class ClientProxy: ClientProxyProtocol { return } - if appSettings.clientPausingAndResumingEnabled { - do { - MXLog.info("Resuming client") - try await client.resume() - } catch { - MXLog.error("Failed resuming client with error: \(error)") - } - } - - MXLog.info("Starting sync") - await syncService.start() - - // If we are using OAuth we want to cache the account management URL in volatile memory on the SDK side. - // To avoid the cache being invalidated while the app is backgrounded, we cache at every sync start. - await cacheAccountURL() + await transitionServices(to: .running).value } /// A stored task for restarting the sync after a failure. This is stored so that we can cancel @@ -447,27 +438,10 @@ class ClientProxy: ClientProxyProtocol { restartTask = nil } - // Capture the sync service strongly as this method is called on deinit and so the - // existence of self when the Task executes is questionable and would sometimes crash. - // Note: This isn't strictly necessary now given the unwrap above, but leaving the code as - // documentation. SE-0371 will allow us to fix this by using an async deinit. - Task { [syncService] in - defer { - completion?() - } - - MXLog.info("Stopping sync") - await syncService.stop() - MXLog.info("Sync stopped") - - if appSettings.clientPausingAndResumingEnabled { - do { - MXLog.info("Pausing client") - try await client.pause() - } catch { - MXLog.error("Failed pausing client with error: \(error)") - } - } + let task = transitionServices(to: .suspended) + Task { + await task.value + completion?() } } @@ -1084,6 +1058,67 @@ class ClientProxy: ClientProxyProtocol { .store(in: &cancellables) } + // MARK: - Pausing and resuming + + private enum ServiceState { case running, suspended } + + /// The state the sync service and client *should* be in, updated by ``resumeServices()`` and ``pauseServices()``. + private var desiredServiceState: ServiceState = .suspended + + /// Serialises service state transitions so that pause and resume never interleave. Each request chains onto + /// the previous one and the reconcile always drives the SDK to the *latest* desired state, so a late background + /// pause can no longer strand the client suspended after a foreground resume. + private var serviceStateTask: Task? + + @discardableResult + private func transitionServices(to state: ServiceState) -> Task { + desiredServiceState = state + + let previousTask = serviceStateTask + let task = Task { [weak self] in + await previousTask?.value + await self?.reconcileServiceState() + } + serviceStateTask = task + return task + } + + private func reconcileServiceState() async { + switch desiredServiceState { + case .running: + if appSettings.clientPausingAndResumingEnabled { + do { + MXLog.info("Resuming client") + try await client.resume() + } catch { + MXLog.error("Failed resuming client with error: \(error)") + } + } + + MXLog.info("Starting sync") + await syncService.start() + + // If we are using OAuth we want to cache the account management URL in volatile memory on the SDK side. + // To avoid the cache being invalidated while the app is backgrounded, we cache at every sync start. + await cacheAccountURL() + case .suspended: + MXLog.info("Stopping sync") + await syncService.stop() + MXLog.info("Sync stopped") + + if appSettings.clientPausingAndResumingEnabled { + do { + MXLog.info("Pausing client") + try await client.pause() + } catch { + MXLog.error("Failed pausing client with error: \(error)") + } + } + } + } + + // MARK: - Other + private func cacheAccountURL() async { // Calling this function for the first time will cache the account URL in volatile memory for 24 hrs on the SDK. _ = try? await client.accountUrl(action: nil)