KM-17274: purchase intents - #383
Conversation
KM-17274: add purchase intents observers
|
Claude finished @kp-mario-nachbaur's task in 7m 0s —— View job Code review: purchase intents
Nice, focused feature. The refactor of Three things I think are blocking, then some design/robustness points. 🔴 Blocking1. Two 2. 3. Retain cycle in 🟡 Design / robustness4. Observation starts too late and lives in the wrong place. Both
I'd start the observer in 5. Observer lifecycle is single-use but not documented as such. 6. Cancellation branch in 7. No re-entrancy guard on purchases. Neither 8. SK1 fallback can drop the purchase silently. You return 🟢 Nits
Security: nothing concerning. No credentials or secrets logged; the logs are product identifiers and transaction ids, and verification still goes through Test coverageNo tests were added, and the two existing mocks weren't updated (item 1). Worth adding:
Flows to regression-test
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
KB review —
|
dc24aef to
0f4c9c8
Compare
This comment was marked as low quality.
This comment was marked as low quality.
|
Claude finished @kp-mario-nachbaur's task in 4m 16s —— View job Code review: purchase intents — re-review at
|
| # | Item | How |
|---|---|---|
| 1 | Missing purchase(product:) in both test mocks |
Stubs added in PIA VPNTests/Mocks/AccountProviderMock.swift:77 and PIA VPN-tvOSTests/Login/Helpers/AccountProviderMock.swift:98 |
| 2 | tvOS build break | #if !os(tvOS) wraps the whole file. Correct choice: PIALibrary platforms are .iOS(.v15), .tvOS(.v17), .macCatalyst(.v15), and Mac Catalyst maps to the iOS availability domain so #available(iOS 16.4, *) behaves properly there |
| 3 | Retain cycle leaking the whole welcome flow | intents is now bound once and self?. used per-iteration, so no strong self is held for the task's lifetime and deinit can actually run |
| 7 | No re-entrancy guard on purchases | guard !isPurchasing, !isRestoring added to both startPurchaseProcess overloads and to logInWithReceipt |
| — | stop() permanently killing the stream |
continuation.finish() dropped; the consumer's for await now unwinds via task cancellation, and the observer is restartable |
I also verified the #if os(iOS) removal in Client.swift is safe: AccountProvider.listPlanProducts/purchase live under #if os(iOS) || os(tvOS) (AccountProvider.swift:180), Client.store is unconditional (Client.swift:53), and refreshProducts/observeTransactions/dispose are only called from the iOS Bootstrapper — so no tvOS behaviour change.
🟡 Still open
4. Observation starts too late / lives in the wrong place — unchanged. Both PurchaseIntent.intents and SKPaymentQueue.add(_:) are documented as "set up as early as possible at launch". Registering in GetStartedViewController.viewDidLoad means a logged-in user has no observer at all, so a tap on Buy in the App Store produces no feedback whatsoever; and if the welcome flow has pushed login/restore, handlePurchase fires signupViaPurchaseSegue from a VC that isn't topmost. Bootstrapper.swift:182, next to Client.observeTransactions(), is where I'd put this. (inline)
5. start() still isn't idempotent — calling it twice overwrites observeTask (leaking the first, uncancelled task) on 16.4+, or double-registers with SKPaymentQueue on 15–16.3 while stop() removes once. Also no deinit finishing the continuation. (inline)
6. Cancellation branch in start() — logs "Task cancelled" when the real cause is a deallocated self, and calls stop() re-entrantly, writing observeTask off the main actor while start()/stop() are called from it. I confirmed the app target is SWIFT_VERSION = 5.0 and the package sets no swiftSettings, so the compiler won't flag this race. (inline)
8. SK1 fallback can drop the purchase silently — you return false (discarding the queued SK1 payment), so if Product.products(for:) throws or returns empty the user gets nothing. On iOS 15–16.3 this is the only promoted-purchase path. (inline)
🟡 New / smaller points
- Nonisolated access to main-actor state:
self?.purchaseIntentObserveris read from a nonisolatedTaskandPurchaseIntentObserverisn'tSendable. Harmless under Swift 5 mode, an error the day the target moves to Swift 6 (which CLAUDE.md lists as the project's language). Hoistinglet intents = purchaseIntentObserver.purchaseIntentsout of the closure fixes it and reads better. (inline) shouldRestoreInsteadOfPurchase()silently resetsisPurchasing(line 276) — a side-effecting predicate; the call sites read as pure checks. (inline)isPurchasing = falsebeforeperform(segue:)— an intent arriving right after a successful purchase can start a second purchase behind the signup screen.- The two
startPurchaseProcessoverloads are byte-identical apart from one line — worth collapsing into a shared body taking the purchase closure, so the guard/loading/handle sequence can't drift. (inline)
🟢 Nits
@inlinableonDefaultAccountProvider.purchase(product:)still buys nothing and is inconsistent with the rest of the type. (inline)hasIntroOffer: falsenow carries a comment explaining it's unused — good enough for now, though a lookup inClient.store.availableProductswould also validate the incoming identifier is one of ours before starting a purchase.- Redundant
@MainActoron the overloads andTask { @MainActor ... }inconfirmPlan—UIViewControllersubclasses are already main-actor isolated. case .failure(.userCancelled): log.debug(…); break— thebreakis dead after a statement.// TODO: email has always been empty string, find out if we can remove(GetStartedViewController.swift:340) — behaviour is unchanged, but worth a ticket rather than a bare TODO.observePurchaseIntents()is called beforesuper.viewDidLoad(); pre-existing ordering quirk in this file, but the new call inherits it.- Inner
productonPurchaseIntentObserver.swift:84shadows theSKProductparameter with aStoreKit.Product—sk2Productwould read better.
Security: nothing concerning. The new logs carry product identifiers and transaction ids only — no credentials or receipts. Verification still goes through JWS + Product.PurchaseResult in AppStoreProvider.
Test coverage
Still no tests added. The mocks compile but discard the argument, so nothing about the new path is assertable. Worth adding:
PurchaseIntentObserver:start()idempotency, termination on cancellation, and the SK1 → SK2 identifier mapping including the "product not found" path (a StoreKitTest.storekitconfiguration covers the last one).PIA VPNTests: recordpurchase(product:)on the mock and assert forwarding, plus the re-entrancy guards and theshouldRestoreInsteadOfPurchasebranch now shared by both entry points.
Flows to regression-test
- Promoted purchase from the App Store page: (a) app not running, (b) backgrounded, (c) foregrounded on the welcome screen — on iOS 16.4+ and an iOS 15/16.3 device for the SK1 path.
- Purchase intent while already signed in, and with an expired account (expect nothing to happen today — see item 4).
- Purchase intent when the App Store account already owns a subscription → restore prompt.
- Normal plan selection →
confirmPlan→ signup, including cancel, pending (Ask to Buy), and expired-transaction cases. - Restore purchases + login-with-receipt (touched by the shared
shouldRestoreInsteadOfPurchaseand the new!isPurchasingguard). - Repeated entry/exit of the welcome screen — confirm
deinitnow fires and the VC deallocates (Memory Graph). - tvOS signup/purchase flow — sanity check after the
Client.swiftguard removal (tvOS unit tests already pass). - Mac Catalyst:
PurchaseIntentis in scope there via the iOS availability mapping, and the file is no longer excluded — worth one manual pass.
· branch KM-17274-purchase-intents
| guard let self, !Task.isCancelled else { | ||
| log.debug("Task cancelled, stopping") | ||
| self?.stop() | ||
| break | ||
| } |
There was a problem hiding this comment.
Still open from the previous round — the #if !os(tvOS) guard fixed the availability problem, but this guard body has three issues:
- Misleading log: the
elsebranch is also taken whenselfwas deallocated, which then logs "Task cancelled, stopping". - Re-entrant
stop()off the main actor:stop()cancels the very task it's called from and writesobserveTask = nil, whilestart()/stop()are invoked from the main actor (GetStartedViewController).observeTaskis unsynchronized mutable state on a non-Sendableclass → data race. The app target isSWIFT_VERSION = 5.0and the package has noswiftSettings, so the compiler stays silent about it. - Redundant:
for awaitalready unwinds on cancellation, and whoever cancelled is the one that calledstop().
| guard let self, !Task.isCancelled else { | |
| log.debug("Task cancelled, stopping") | |
| self?.stop() | |
| break | |
| } | |
| guard let self else { break } |
| public func stop() { | ||
| observeTask?.cancel() | ||
| observeTask = nil | ||
| SKPaymentQueue.default().remove(self) | ||
| } |
There was a problem hiding this comment.
Good change — dropping continuation.finish() here is what makes the object restartable and lets the consumer's for await terminate purely via task cancellation. Two leftovers:
start()is still not idempotent: calling it twice overwritesobserveTask(leaking the first, uncancelled task) on 16.4+, or addsselftoSKPaymentQueuetwice on 15–16.3 whilestop()removes it once.guard observeTask == nil else { return }covers the SK2 side; the SK1 side needs its own flag since it doesn't setobserveTask.- No
deinitfinishing the continuation. Today the VC cancels its task explicitly, so nothing hangs — but if a future consumer forgets, thefor awaitnever ends. Adeinit { continuation.finish() }(plusstop()) would make that safe by construction.
| private func observePurchaseIntents() { | ||
| purchaseIntentObserver.start() | ||
| purchaseIntentsTask = Task { [weak self] in | ||
| guard let intents = self?.purchaseIntentObserver.purchaseIntents else { return } | ||
| for await product in intents { | ||
| if Task.isCancelled { break } | ||
| log.debug("Purchase intent received for product id: \(product.identifier)") | ||
| await self?.startPurchaseProcess(withProduct: product) | ||
| } | ||
| } |
There was a problem hiding this comment.
✅ Retain cycle resolved — binding intents once and using self?. inside the loop means no strong self is held for the task's lifetime, so deinit can now actually run and cancel the task. Nice.
Two remaining notes:
- Nonisolated access to main-actor state.
self?.purchaseIntentObserveris read from a nonisolatedTask, andPurchaseIntentObserverisn'tSendable. UnderSWIFT_VERSION = 5.0this is at most a warning, but it will be an error whenever the target moves to Swift 6 mode. Hoisting the stream out before the task avoids both the optional dance and the isolation crossing:
| private func observePurchaseIntents() { | |
| purchaseIntentObserver.start() | |
| purchaseIntentsTask = Task { [weak self] in | |
| guard let intents = self?.purchaseIntentObserver.purchaseIntents else { return } | |
| for await product in intents { | |
| if Task.isCancelled { break } | |
| log.debug("Purchase intent received for product id: \(product.identifier)") | |
| await self?.startPurchaseProcess(withProduct: product) | |
| } | |
| } | |
| private func observePurchaseIntents() { | |
| purchaseIntentObserver.start() | |
| let intents = purchaseIntentObserver.purchaseIntents | |
| purchaseIntentsTask = Task { [weak self] in | |
| for await product in intents { | |
| if Task.isCancelled { break } | |
| log.debug("Purchase intent received for product id: \(product.identifier)") | |
| await self?.startPurchaseProcess(withProduct: product) | |
| } | |
| } | |
| } |
- Timing still worries me (top-level item 4 from the last round, unaddressed): both
PurchaseIntent.intentsandSKPaymentQueue.add(_:)are documented as "set up as early as possible in launch". Registering inviewDidLoadof the welcome screen means a logged-in user has no observer at all, so tapping Buy on the App Store page produces zero feedback.Bootstrapper.swift:182(next toClient.observeTransactions()) is where I'd put this.
| private func startPurchaseProcess(withPlan plan: PurchasePlan) async { | ||
| guard !isPurchasing, !isRestoring else { | ||
| log.debug("Ignoring purchase request, purchase or restore already in progress") | ||
| return | ||
| } | ||
| isPurchasing = true | ||
| self.handleLoadingState() | ||
| handleLoadingState() | ||
| if await shouldRestoreInsteadOfPurchase() { return } | ||
| let result = await config.accountProvider.purchase(plan: plan.plan) | ||
| isPurchasing = false | ||
| handleLoadingState() | ||
| await handlePurchase(result: result) | ||
| } | ||
|
|
||
| Task { [weak self] in | ||
| guard let self else { return } | ||
| @MainActor | ||
| private func startPurchaseProcess(withProduct product: any InAppProduct) async { | ||
| guard !isPurchasing, !isRestoring else { | ||
| log.debug("Ignoring purchase request, purchase or restore already in progress") | ||
| return | ||
| } | ||
| isPurchasing = true | ||
| handleLoadingState() | ||
| if await shouldRestoreInsteadOfPurchase() { return } | ||
| let result = await config.accountProvider.purchase(product: product) | ||
| isPurchasing = false | ||
| handleLoadingState() | ||
| await handlePurchase(result: result) | ||
| } | ||
|
|
There was a problem hiding this comment.
✅ Re-entrancy guards added on both overloads and on logInWithReceipt — that closes the "promoted purchase arrives mid-purchase" hole.
Two follow-ups now that the two bodies are byte-identical apart from one line:
-
shouldRestoreInsteadOfPurchase()silently owns theisPurchasingreset. Readingif await shouldRestoreInsteadOfPurchase() { return }at the call site, it isn't obvious the flag gets cleared inside (line 276) — a side-effecting predicate. It'd be clearer to have the predicate be pure and reset/handleLoadingState()/alertExistingEntitlement()at the call site, or rename it to something that admits the side effect. -
isPurchasing = falseis set beforeperform(segue:)inhandlePurchase, so an intent arriving right after a successful purchase can kick off a second purchase behind the signup screen. Holding the flag until signup completes (or gating onpresentedViewController == nil) would close that.
Also, the two overloads could collapse into one shared body:
private func startPurchaseProcess(_ purchase: () async -> Result<any InAppTransaction, ClientError>) async { ... }with the two public entry points just supplying the closure. Optional, but it removes a copy-paste pair that has to stay in sync.
Nit: @MainActor on both is redundant — UIViewController subclasses are already main-actor isolated, as are handlePurchase and shouldRestoreInsteadOfPurchase, and so is the Task { @MainActor [weak self] in ... } in confirmPlan.
1 participant
Start the purchasing process from the App Store via "deep link" or PurchaseIntents.