fix: Incremental sync failing to resume - WPB-28101 - #5179
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new suspension/resume state introduces a likely data race (concurrent resume()/suspend() access to non-isolated properties) that can still lead to missed or duplicated resumes under real multi-task call patterns.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses a sync lifecycle edge case in wire-ios-sync-engine where an in-progress suspension (e.g., app backgrounding mid-sync) can prevent a subsequent foreground resume from restarting incremental sync.
Changes:
- Queue
resume()requests that occur whilesuspend()is still running, and trigger the resume once suspension completes. - Add a unit test covering “resume during suspension” to prevent regressions.
File summaries
| File | Description |
|---|---|
| wire-ios-sync-engine/Source/Synchronization/SyncAgent.swift | Adds “pending resume” state to defer resumes until an ongoing suspension finishes. |
| wire-ios-sync-engine/Tests/Source/Synchronization/SyncAgentTests.swift | Adds a new async test that simulates a long-running suspension and verifies resume triggers only after completion. |
Review details
Suppressed comments (1)
wire-ios-sync-engine/Source/Synchronization/SyncAgent.swift:212
pendingResumeis consumed without being cleared, so (a) a subsequent call tosuspend()could incorrectly see stale state until its initial reset runs, and (b) it’s easier to accidentally trigger duplicate resumes if additional logic later re-checks the flag. Clearing the pending request after capturing it makes the intent explicit and avoids reusing the same request.
isSuspendingSync = false
if let pendingResume {
resume(callEventsOnly: pendingResume.callEventsOnly)
}
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results1 139 tests 1 139 ✅ 1m 26s ⏱️ Results for commit 762d11e. ♻️ This comment has been updated with latest results. Summary: workflow run #34336161079 |
|
There was a problem hiding this comment.
🟡 Changes recommended
The new unit test has a scheduling race that can make it flaky/non-deterministic without an explicit yield before asserting invocation counts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
netbe
left a comment
There was a problem hiding this comment.
left some suggestions.
question: Does the /access call come from WireNetwork or WireTransport ? I still would like to know why the call would fail resuming the sync: it might be an issue with cancellation of renewing the access token code
@netbe The /access call is via WireNetwork. Looking at the The more general issue here is:
To be clear this PR is definitely a patch to address the immediate issue. |
|
Playground build here: https://github.com/wireapp/wire-ios/actions/runs/34341179123 |
Issue
This PR fixes an issue where an unfinished suspension of sync when going to the background interferes and prevents sync from resuming when returning to the foreground. This has been seen to happen when the device is backgrounded when performing a request to
/access?client_id=<some client id>as part of an incremental sync that is still not complete / cancelled when resuming by returning to the foreground.This PR attempts to fix this by making it so that if there is an ongoing
suspend()whenresume()is called,resume()is deferred to after the suspension is complete.I don't expect this to fix all issues here. Larger changes are probably necessary. But I hope this improves the situation somewhat.
Testing
These steps require a proxy app. Setup is complex so first try the following steps with a known broken version to ensure you can reproduce the issue.
Checklist
[WPB-XXX].