`GetCustomerInfo` posts receipts if there are pending transactions #2533

NachoSoto · 2023-05-23T20:46:04Z

This fixes the last edge-case for offline entitlements. Solves the following scenario:

func testPurchaseWhileServerIsDownPostsReceiptWhenForegroundingApp() async throws {
    let logger = TestLogHandler()

    // 1. Purchase while server is down
    self.serverDown()
    try await self.purchaseMonthlyProduct()

    logger.verifyMessageWasNotLogged("Finishing transaction")

    // 2. Server is back
    self.serverUp()

    // 3. Request current CustomerInfo
    let info1 = try await Purchases.shared.customerInfo()
    try await self.verifyEntitlementWentThrough(info1)

    // 4. Ensure transaction is finished
    logger.verifyMessageWasLogged("Finishing transaction", level: .info)

    // 5. Restart app
    Purchases.shared.invalidateCustomerInfoCache()
    await self.resetSingleton()

    // 6. To ensure (with a clean cache) that the receipt was posted
    let info2 = try await Purchases.shared.customerInfo()
    try await self.verifyEntitlementWentThrough(info2)
}

This race condition happened when fetching CustomerInfo after we had computed an offline CustomerInfo with a purchase: if the server is still not aware of the purchase, then we lose that until some arbitrary amount of time when StoreKit might decide to send the pending transactions in the queue.
For that reason, instead of waiting, we proactively check now whenever CustomerInfoManager fetches new info.

The refactors #2540 and #2542 where required to break a retain cycle.
Also thanks to #2534 we know for sure that there aren't any cycles.

Note that this is an iOS 15.0+ only feature. It would be useful to better ensure consistency in older versions, but it's not strictly required, as those versions don't support offline entitlements either.

While working on #2533, I knew that my proof of concept had a retain cycle. I however was very confused when it wasn't detected by this mechanism introduced first introduced in #2104. Turns out that this had been wrong the whole time: the current implementation, as explained by the comment, was executing those blocks in LIFO order, which meant that the references were being set to `nil` before the assertions could be created. This meant that by the time the `except { [weak purchases = self.purchases] ... }` lines were called, `self.purchases` itself was `nil`, so nothing was being checked. This simplifies the implementation by inlining the checks, and saving a `weak` reference before clearing the singleton and all the local references. Luckily there was only one failing test that had to do with how `Purchases` was being created, and implementation details of `expectFatalError`. To avoid this, I changed the test to use the `Purchases.init` method directly.

@available

…gate` transaction handling While working on #2533 I noticed that the implementation for `StoreKit2TransactionListenerDelegate` was unnecessarily complex: ```swift @available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *) extension PurchasesOrchestrator: StoreKit2TransactionListenerDelegate { func storeKit2TransactionListener( _ listener: StoreKit2TransactionListener, updatedTransaction transaction: StoreTransactionType ) async throws { let isRestore = self.systemInfo.observerMode _ = try await self.syncPurchases(receiptRefreshPolicy: .always, isRestore: isRestore, initiationSource: .queue) await Async.call { completion in self.finishTransactionIfNeeded(transaction) { @mainactor in completion(()) } } } } ``` Instead of only posting the one transaction, we were calling `syncPurchases`, which is inconsistent with how we handle SK1 transactions. The new logic in #2533 will also make use of this single-transaction handling code, so it makes sense to unify this as well. This only required an overload to specify the `InitiationSource`, which uncovered one incorrect `isRestore` parameter in one of our tests. I verified that this is expected based on other tests, and the implementation of `initiationSource(for productIdentifier:restored:)`, as well as checking with @antoniobg.

While working on #2533, I knew that my proof of concept had a retain cycle. I however was very confused when it wasn't detected by this mechanism introduced first introduced in #2104. Turns out that this had been wrong the whole time: the current implementation, as explained by the comment, was executing those blocks in LIFO order, which meant that the references were being set to `nil` before the assertions could be created. This meant that by the time the `except { [weak purchases = self.purchases] ... }` lines were called, `self.purchases` itself was `nil`, so nothing was being checked. This simplifies the implementation by inlining the checks, and saving a `weak` reference before clearing the singleton and all the local references. Luckily there was only one failing test that had to do with how `Purchases` was being created, and implementation details of `expectFatalError`. To avoid this, I changed the test to use the `Purchases.init` method directly.

codecov · 2023-05-26T20:53:12Z

Codecov Report

❗ No coverage uploaded for pull request base (transaction-poster-3@397edda). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 619d960 differs from pull request most recent head 7c3168c. Consider uploading reports for the commit 7c3168c to get more accurate results

@@                   Coverage Diff                   @@
##             transaction-poster-3    #2533   +/-   ##
=======================================================
  Coverage                        ?   87.96%           
=======================================================
  Files                           ?      201           
  Lines                           ?    13931           
  Branches                        ?        0           
=======================================================
  Hits                            ?    12254           
  Misses                          ?     1677           
  Partials                        ?        0

This improves an upcoming log from #2533: ``` DEBUG: ℹ️ Found unfinished transaction, will post receipt in lieu of fetching CustomerInfo: <StoreTransaction: identifier="9CB3E6CE-A6F6-41EC-B25D-FE0CCE5783F6" product="" date="2023-05-26 21:07:55 +0000" quantity=1 > ```

Necessary refactor for #2533. This allows `CustomerInfoManager` to have an instance of `TransactionPosterType` instead of the whole `PurchasesOrchestrator`. Essentially this becomes the hierarchy now: ![IMG_B8C56C11B7E5-1](https://github.com/RevenueCat/purchases-ios/assets/685609/36f5c30b-fc82-4590-b574-ce6d95eef13f) ### Changes: - Created stateless `TransactionPoster` - Extracted `handlePurchasedTransaction` into `TransactionPoster` - The new implementation there is _mostly_ the same, with the one difference that it's stateless. Instead of holding `purchaseCompleteCallbacksByProductID`, that's done by `PurchasesOrchestrator`. The methods there all take a callback, so they're easier to use and don't require keeping a callback around. - Created `PurchaseSource` to abstract the combo of `isRestore` and `InitiationSource`. This is still messy using the deprecated `allowSharingAppStoreAccount`, but at least that's abstracted out and the new `TransactionPoster` won't deal with that deprecated property. - I didn't mock `TransactionPoster`, which means that all existing tests that check `PurchasesOrchestrator` continue working the same way. This was on purpose to keep this refactor as simple as possible. - This supersedes #2536: the change made there was now possible here without any integration test failures, thanks to the fact that we can process transactions independently without messing with the `purchaseCompleteCallbacksByProductID` state now. ### Future improvements This is just a start, there's a lot more than can be done to simplify the `PurchasesOrchestrator` monster. The main thing would be to remove all the custom code for `restorePurchases`, which is largely the same as what's in `TransactionPoster` now. ### Notes: Just posting this here for posteriority. I had to trace the call hierarchy for both SK1 and SK2. This "block" is what's now in `TransactionPoster`: ![IMG_C60274393F7F-1](https://github.com/RevenueCat/purchases-ios/assets/685609/5a7377f0-8301-474f-a774-a90b6e232bc3)

All these tests were calling the method without waiting for `completion`. This breaks things in an upcoming PR (#2533) because it's no longer synchronous, but also this avoids leaving things running behind when the tests are done.

… to finish All these tests were calling the method without waiting for `completion`. This breaks things in an upcoming PR (#2533) because it's no longer synchronous, but also this avoids leaving things running behind when the tests are done.

tonidero

Just a question but looks great! I think we should also implement this behavior in Android soon

tonidero · 2023-05-29T07:18:49Z

Sources/Identity/CustomerInfoManager.swift

+    private static let sourceForUnfinishedTransaction: PurchaseSource = .init(
+        isRestore: false,
+        // This might have been in theory a `.purchase`. The only downside of this is that the server
+        // won't validate that the product is present in the receipt.


Hmm can you explain more in what situation this transaction could originate from a purchase? If we are in the middle of posting the transaction after a purchase, this should wait for that I guess?

Server offline

Purchase fails

Compute customer info (but transaction still in queue)

Next time you launch the app, server is back up

Pending transaction -> post receipt

Technically that was a .purchase when it was done originally, but we're sending it as .queue now.

Ahh right, that would also happen currently if a POST receipt fails and it's retried later on right? But yeah, that makes sense 👍

… parameters (#2542) Follow up to #2540. This was necessary to ensure that we can implement #2533 without reference cycles. This was the hierarchy before this change: ![IMG_B25ECE86C2F9-1](https://github.com/RevenueCat/purchases-ios/assets/685609/70e1d939-8fa5-424c-8673-bb519f386118) The red arrows are what this PR removes. ## Changes: - `TransactionPoster` no longer has references to `IdentityManager` and `Attribution` - `markSyncedIfNeeded` is no longer in `TransactionPoster`, moved back to `PurchasesOrchestrator` - Changed `TransactionPoster`'s callback to return `Result<CustomerInfo, BackendError>`, providing greater flexibility for users to be able to read the specific errors. - Extracted `PurchasedTransactionData` to be reduce the number of parameters passed around. This removed the need for 2 `swiftlint:disable:next function_parameter_count`. This is the majority of this diff. It was helpful to combine in this PR because I needed to add another parameter to the flow: `unsyncedAttributes`, since `TransactionPoster` could no longer read these directly.

@NachoSoto

**This is an automatic release.** ### New Features * `Offline Entitlements`: use offline-computed `CustomerInfo` when server is down (#2368) via NachoSoto (@NachoSoto) ### Bugfixes * `AppleReceipt.debugDescription`: don't pretty-print JSON (#2564) via NachoSoto (@NachoSoto) * `SK2StoreProduct`: fix crash on iOS 12 (#2565) via NachoSoto (@NachoSoto) * `GetCustomerInfo` posts receipts if there are pending transactions (#2533) via NachoSoto (@NachoSoto) ### Performance Improvements * `PurchasedProductsFetcher`: cache current entitlements (#2507) via NachoSoto (@NachoSoto) * Performance: new check to ensure serialization / deserialization is done from background thread (#2496) via NachoSoto (@NachoSoto) ### Dependency Updates * Bump fastlane from 2.212.2 to 2.213.0 (#2544) via dependabot[bot] (@dependabot[bot]) ### Other Changes * `CustomerInfoManager`: post all unfinished transactions (#2563) via NachoSoto (@NachoSoto) * `PostReceiptOperation`: added ability to also post `AdServices` token (#2566) via NachoSoto (@NachoSoto) * `Offline Entitlements`: improved computation log (#2562) via NachoSoto (@NachoSoto) * Added `TransactionPoster` tests (#2557) via NachoSoto (@NachoSoto) * Refactored `TransactionPoster`: removed 2 dependencies and abstracted parameters (#2542) via NachoSoto (@NachoSoto) * `CustomerInfoManagerTests`: wait for `getAndCacheCustomerInfo` to finish (#2555) via NachoSoto (@NachoSoto) * `StoreTransaction`: implemented `description` (#2556) via NachoSoto (@NachoSoto) * `Backend.ResponseHandler` is now `@Sendable` (#2541) via NachoSoto (@NachoSoto) * Extracted `TransactionPoster` from `PurchasesOrchestrator` (#2540) via NachoSoto (@NachoSoto) * `enableAdServicesAttributionTokenCollection`: added integration test (#2551) via NachoSoto (@NachoSoto) * `AttributionPoster`: replaced hardcoded strings with constants (#2548) via NachoSoto (@NachoSoto) * `DefaultDecodable`: moved to `Misc/Codable/DefaultDecodable.swift` (#2528) via NachoSoto (@NachoSoto) * `CircleCI`: specify device to run `backend_integration_tests` (#2547) via NachoSoto (@NachoSoto) * Created `StoreKit2TransactionFetcher` (#2539) via NachoSoto (@NachoSoto) * Fix load shedder integration tests (#2546) via Josh Holtz (@joshdholtz) * Fix doc on `Offering.getMetadataValue` (#2545) via Josh Holtz (@joshdholtz) * Extracted and tested `AsyncSequence.extractValues` (#2538) via NachoSoto (@NachoSoto) * `Offline Entitlements`: don't compute offline `CustomerInfo` when purchasing a consumable products (#2522) via NachoSoto (@NachoSoto) * `OfflineEntitlementsManager`: disable offline `CustomerInfo` in observer mode (#2520) via NachoSoto (@NachoSoto) * `BasePurchasesTests`: fixed leak detection (#2534) via NachoSoto (@NachoSoto) * `PurchaseTesterSwiftUI`: added `ProxyView` to `iOS` (#2531) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: removed `AppStore.sync` call (#2521) via NachoSoto (@NachoSoto) * `PurchaseTesterSwiftUI`: added new window on Mac to manage proxy (#2518) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: added log if fetching purchased products is slow (#2515) via NachoSoto (@NachoSoto) * `Offline Entitlements`: disable for custom entitlements mode (#2509) via NachoSoto (@NachoSoto) * `Offline Entitlements`: fixed iOS 12 tests (#2514) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: don't throw errors if purchased products were found (#2506) via NachoSoto (@NachoSoto) * `Offline Entitlements`: allow creating offline `CustomerInfo` with empty `ProductEntitlementMapping` (#2504) via NachoSoto (@NachoSoto) * `Offline Entitlements`: integration tests (#2501) via NachoSoto (@NachoSoto) * `CustomerInfoManager`: don't cache offline `CustomerInfo` (#2378) via NachoSoto (@NachoSoto) * `DangerousSettings`: debug-only `forceServerErrors` (#2486) via NachoSoto (@NachoSoto) * `CocoapodsInstallation`: fixed `Xcode 14.3.0` issue (#2489) via NachoSoto (@NachoSoto) * `CarthageInstallation`: removed workaround (#2488) via NachoSoto (@NachoSoto) --------- Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>

Similar to #2786 and #2777. This test was verifying `getCustomerInfoCallCount`, but that gets updated asynchronously since #2533. The correct version waits for it to be updated, then ensures that it doesn't get updated again after logging out.

Similar to #2786 and #2777. This test was verifying `getCustomerInfoCallCount`, but that gets updated asynchronously since #2533. The correct version waits for it to be updated, then ensures that it doesn't get updated again after logging out. Fixes https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/12762/workflows/a7e8c4ac-cecb-4c25-90ff-4390ed226bc4/jobs/88794

NachoSoto added the pr:fix A bug fix label May 23, 2023

NachoSoto requested a review from a team May 23, 2023 20:46

NachoSoto mentioned this pull request May 23, 2023

BasePurchasesTests: fixed leak detection #2534

Merged

NachoSoto force-pushed the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch from 06af69b to 77ddcc8 Compare May 23, 2023 23:53

NachoSoto mentioned this pull request May 24, 2023

PurchasesOrchestrator: simplified StoreKit2TransactionListenerDelegate transaction handling #2536

Closed

Base automatically changed from integration-test-failure-exception to main May 24, 2023 16:51

This was referenced May 24, 2023

Offline Entitlements: don't compute offline CustomerInfo when purchasing a consumable products #2522

Merged

[AUTOMATIC] Release/4.20.0 #2537

Closed

NachoSoto force-pushed the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch 4 times, most recently from 973d424 to 65182be Compare May 24, 2023 20:04

NachoSoto changed the base branch from main to transaction-fetcher-2 May 24, 2023 20:05

NachoSoto force-pushed the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch from 65182be to e00ce02 Compare May 24, 2023 20:13

This was referenced May 24, 2023

Extracted TransactionPoster from PurchasesOrchestrator #2540

Merged

Refactored TransactionPoster: removed 2 dependencies and abstracted parameters #2542

Merged

NachoSoto force-pushed the transaction-fetcher-2 branch from 2e46a23 to e689b27 Compare May 25, 2023 17:01

Base automatically changed from transaction-fetcher-2 to main May 25, 2023 17:18

NachoSoto force-pushed the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch from e00ce02 to 72d979a Compare May 25, 2023 21:06

NachoSoto changed the base branch from main to transaction-poster-3 May 25, 2023 21:06

NachoSoto force-pushed the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch 3 times, most recently from 4fb32c2 to 9a6dba6 Compare May 25, 2023 22:33

NachoSoto mentioned this pull request May 26, 2023

StoreTransaction: implemented description #2556

Merged

NachoSoto force-pushed the transaction-poster-3 branch from 34de8f5 to 55a39c0 Compare May 26, 2023 22:07

NachoSoto force-pushed the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch from 07fef8b to 619d960 Compare May 26, 2023 22:08

NachoSoto force-pushed the transaction-poster-3 branch from 55a39c0 to 397edda Compare May 26, 2023 22:57

NachoSoto force-pushed the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch from 619d960 to 7c3168c Compare May 26, 2023 22:58

tonidero approved these changes May 29, 2023

View reviewed changes

Base automatically changed from transaction-poster-3 to main May 30, 2023 19:16

NachoSoto added 2 commits May 30, 2023 12:17

[WIP] GetCustomerInfo posts receipts if there are pending transactions

2453809

WIP

0a505fc

NachoSoto force-pushed the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch from 7c3168c to 0a505fc Compare May 30, 2023 19:17

NachoSoto enabled auto-merge (squash) May 30, 2023 19:23

NachoSoto mentioned this pull request May 30, 2023

CustomerInfoManager: post all unfinished transactions #2563

Merged

NachoSoto merged commit de47c57 into main May 30, 2023

NachoSoto deleted the nacho/sdk-3133-ios-if-sdk-detects-pending-transactions-in-the-queue-when branch May 30, 2023 19:38

This was referenced May 31, 2023

[AUTOMATIC] Release/4.21.0 #2569

Closed

[AUTOMATIC] Release/4.21.0 #2571

Merged

NachoSoto mentioned this pull request May 31, 2023

Release/4.20.1 #2572

Closed

NachoSoto mentioned this pull request Jul 12, 2023

Tests: fixed another flaky test #2795

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`GetCustomerInfo` posts receipts if there are pending transactions #2533

`GetCustomerInfo` posts receipts if there are pending transactions #2533

NachoSoto commented May 23, 2023 •

edited

Loading

codecov bot commented May 26, 2023 •

edited

Loading

tonidero left a comment

tonidero May 29, 2023

NachoSoto May 30, 2023

tonidero May 31, 2023

GetCustomerInfo posts receipts if there are pending transactions #2533

GetCustomerInfo posts receipts if there are pending transactions #2533

Conversation

NachoSoto commented May 23, 2023 • edited Loading

codecov bot commented May 26, 2023 • edited Loading

Codecov Report

tonidero left a comment

Choose a reason for hiding this comment

tonidero May 29, 2023

Choose a reason for hiding this comment

NachoSoto May 30, 2023

Choose a reason for hiding this comment

tonidero May 31, 2023

Choose a reason for hiding this comment

`GetCustomerInfo` posts receipts if there are pending transactions #2533

`GetCustomerInfo` posts receipts if there are pending transactions #2533

NachoSoto commented May 23, 2023 •

edited

Loading

codecov bot commented May 26, 2023 •

edited

Loading