chore: Add coordinator to prevent push delivery metrics loss#951
Draft
mahmoud-elmorabea wants to merge 1 commit intomainfrom
Draft
chore: Add coordinator to prevent push delivery metrics loss#951mahmoud-elmorabea wants to merge 1 commit intomainfrom
mahmoud-elmorabea wants to merge 1 commit intomainfrom
Conversation
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. |
mahmoud-elmorabea
changed the title
SDK binary size reports 📊SDK binary size of this PRSDK binary size diff report vs. main branch |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } else { | ||
| // Fallback to original behavior if no coordinator | ||
| RichPushRequestHandler.shared.stopAll() | ||
| } |
There was a problem hiding this comment.
Bug: Stale Coordinator Causes Incorrect Timeout Handling
The currentCoordinator isn't cleared when autoTrackPushEvents is false or after a coordinator successfully completes its operations. This can cause serviceExtensionTimeWillExpire() to operate on a stale coordinator from a previous push, leading to incorrect timeout handling, and may also result in unnecessary memory usage.
Contributor
There was a problem hiding this comment.
this looks like a valid concern, we should set currentCoordinator = nil on didRecieve
Shahroz16
reviewed
Sep 1, 2025
Contributor
Shahroz16
left a comment
Shahroz16
left a comment
There was a problem hiding this comment.
i think we need to add tests for this Coordinator race/timeout cases.
| } else { | ||
| // Fallback to original behavior if no coordinator | ||
| RichPushRequestHandler.shared.stopAll() | ||
| } |
Contributor
There was a problem hiding this comment.
this looks like a valid concern, we should set currentCoordinator = nil on didRecieve
Contributor
There was a problem hiding this comment.
i still see a reference in MessagingInApp can we remove that too, please
| private func callContentHandler(with content: UNNotificationContent, reason: String) { | ||
| logger.info("NSE operations complete (\(reason)). Calling content handler.") | ||
|
|
||
| contentHandler(content) |
Contributor
There was a problem hiding this comment.
should this be done on main thread? did you check with wrappers? they often require NSE operation to be done on main
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why?
Some delivery metrics are lost because we don't wait for that request to finish. The previous implementation had two independent async operations (metrics tracking and rich push processing) and only rich push processing called
contentHandlerupon completion. When rich push is faster, it would callcontentHandlerand terminate the NSE, killing the in-flight metrics request even when plenty of NSE time remained.How we solve?
Add
NSEOperationCoordinatorto wait for both async calls or NSE call that it will expire. The coordinator ensurescontentHandleris called exactly once only after:serviceExtensionTimeWillExpireis called by iOSThis maximizes delivery metrics success by using the full available NSE execution time instead of terminating prematurely when only one operation completes.
Note: Moved
Synchronized<T>utility to Common module for reuse