Repository navigation
feat(apple): unlink background topics a later launch no longer subscribes to - #2022
Conversation
…ibes to Topic subscribers outlive the process so APNs can wake a killed app, but a launch that stops subscribing to a topic, such as after an app update, left its subscriber on the server, and every message on it woke the device with a placeholder the extension could not fill. Push now keeps the topics it linked per target in the shared store. 30 seconds after the device registers, once per launch, it unlinks the saved topics no background subscription of the user claims. A topic subscribed later is linked again.
🔵 Tier A · Mergeable after minor fixes
This pull request persists APNs-linked topics per target and reconciles them after a launch, removing stale subscribers while retaining topics claimed by live background subscriptions. It also deletes a registered target when no background subscriptions remain and ensures a topic claimed during an unlink is linked again. The tests cover linked-topic persistence, target registration tracking, and stale-topic selection. Latest changes: The new commits schedule reconciliation from any subscription, delete registered targets when no background claims remain, and re-link a topic if a same-user Push claims it while an unlink is in flight.
Note Not approving while a bug finding is open: A new background claim can be lost during target deletion
Fix with agent prompt### Issue 1
templates/apple/Sources/Services/Push.swift.twig:1453
**A new background claim can be lost during target deletion**
This branch decides there are no claims before awaiting `deleteTarget`, but a background subscribe can begin while that DELETE is in flight. If its target upsert completes first, the DELETE can remove the target after `register` has already finished, leaving the new subscription without APNs delivery until another registration occurs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 2
✅ Fixed since the last review · 2
Reviewed the commits since |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
…recheck every Push after an unlink A launch that dropped every background subscription never reconciled, so the target and its subscribers stayed and kept waking the device. Every subscribe now schedules the launch's reconcile, the store records the targets this install registered, and with no background subscription left the reconcile deletes the target, as close() does. An unlink re-linked the topic only when the Push that unlinked it still claimed it; a sibling Push of the user claiming it during the delete lost its link. The check now covers every live Push of the user.
The findings that requested changes are resolved. See the summary comment for what is still open.
d38eecf
into
feat/apple-push-apns
| guard let push = pushes.first(where: { !$0.backgroundTopics().isEmpty }) else { | ||
| // No background subscription this launch: the target and its subscribers go, as on close(). | ||
| if apnsLinkedTopics.isRegistered(targetId), let push = fallback ?? pushes.first { | ||
| await deleteTarget(push, targetId: targetId) |
There was a problem hiding this comment.
A new background claim can be lost during target deletion
This branch decides there are no claims before awaiting deleteTarget, but a background subscribe can begin while that DELETE is in flight. If its target upsert completes first, the DELETE can remove the target after register has already finished, leaving the new subscription without APNs delivery until another registration occurs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: templates/apple/Sources/Services/Push.swift.twig
Line: 1453
Comment:
**A new background claim can be lost during target deletion**
This branch decides there are no claims before awaiting `deleteTarget`, but a background subscribe can begin while that DELETE is in flight. If its target upsert completes first, the DELETE can remove the target after `register` has already finished, leaving the new subscription without APNs delivery until another registration occurs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · concurrency · Reply if this doesn't apply.
There was a problem hiding this comment.
I don’t see that fix in the revision available here: reconcile checks for claims before awaiting deletion, and deleteTarget then performs the DELETE without re-registering any claims that appeared in the meantime. If registration’s upsert finishes before the DELETE, the target can still be removed. (reconcile and deleteTarget)
Please make sure commit 966125e16 is included in the reviewed stack; I’ll recheck once it’s visible here.
1 participant
What does this PR do?
Removes topic subscribers that an earlier launch of the app created and a later launch no longer uses. Without this, every message on such a topic wakes the device with an empty "New message" banner.
Stacked on #2000.
The problem
A background subscription creates a subscriber row on the server: "this device's target wants
news". The server decides whether to push through APNs from that row, so a killed app can still be woken. The row has to outlive the process.If version 1 of an app subscribed to
newsin the background and version 2 doesn't:news, so nothing deletes it.newsmessage still makes the server push the placeholder to the device.unsubscribe,update(background: false),close()and sign-out already removed rows within a launch. Only a topic that quietly disappears between launches was left behind, because the SDK kept its record of linked topics in memory only.The fix
Push.LinkedTopics), per target, in the store shared with the extension, under the same cross-process lock.404counts as removed).close(), clears the target's entry.Push.reconcileDelay),Pushtakes every livePushof the user and unlinks each saved topic that none of them has as a background subscription. It never runs in the extension.Test Plan
Unit tests (24 in total, 2 new):
Lint, macOS, iOS Simulator and Linux (
swift:6.1-jammy, from the e2e fixture) builds; Generation suite and Twig lint.iOS Simulator against a real local Appwrite, its broker and the APNs sandbox, with topic
news:newssubscribers on the servernewsin the backgroundnews, at ~12 snewswith the app terminatednewsonly after 40 s: ~34 s / ~48 snewswhile it is claimed, app terminatedRelated PRs and Issues