Only Retry POST Receipt Paths for 429 #4107

fire-at-will · 2024-07-24T16:25:43Z

Description

This PR modifies the HTTPClient's retry logic to only perform retries on requests to the /v1/receipts URL path that result in a 429 status code.

Testing

Updated existing tests to set the isRetryable field correctly
Added unit tests to HTTPClient.Request to ensure that the isRetryable field is being set properly
Wrote new unit & integration tests to ensure that we don't retry requests to unsupported endpoints (like /v1/subscribers/identify)

fire-at-will · 2024-07-24T16:54:25Z

Sources/Networking/HTTPClient/HTTPClient.swift

-            isRetryable = Bool(isRetryableString.lowercased()) ?? true
-        } else {
-            isRetryable = true
+        let doesServerAllowRetryString = urlResponse.value(forHTTPHeaderField: ResponseHeader.isRetryable.rawValue)


No change in the logic, just renaming isRetryable to doesServerAllowRetry to be more clear

fire-at-will · 2024-07-24T17:01:54Z

@RCGitBot please test

fire-at-will · 2024-07-24T17:37:10Z

Some of the test checks look a little funny right now because it took me a moment to figure out how to run all of the tests, I'll make sure they're all good to go before merging 💪

fire-at-will · 2024-07-24T17:59:49Z

Tests/BackendIntegrationTests/OtherIntegrationTests.swift

@@ -173,7 +175,7 @@ class OtherIntegrationTests: BaseBackendIntegrationTests {
        try AvailabilityChecks.iOS15APIAvailableOrSkipTest()

        let result = try await self.purchases.productEntitlementMapping().entitlementsByProduct
-        expect(result).to(haveCount(18))
+        expect(result).to(haveCount(21))


I'm not sure when (the audit logs aren't loading for me), but we now have 21 iOS products in the StoreKit Integration Tests app instead of 18

I'm looking into this a little more to confirm why this has changed

The most recently added product to the project was on June 25, so this isn't an issue with a new product being present. Continuing to dig 👀

The test is currently failing in main with the value 18, but passed during yesterday's release pipeline 🧐

https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/20914/workflows/68cd311d-9aec-47a8-8f55-889bef8bdabc

@RevenueCat/coresdk thoughts?

Josh and I took a look at some network logs and found that the backend was returning 18 mappings on Tuesday when the test passed, but is now returning 21 in the network calls to /v1/product_entitlement_mapping. This is happening on this branch, main, and the 4.32.2 release. We now believe that 21 is the correct number and are following up with the backend team to investigate what caused the change to take so long to appear.

aboedo

looking good so far

aboedo · 2024-07-24T20:33:01Z

Sources/Networking/HTTPClient/HTTPClient.swift

+        var doesServerAllowRetry: Bool = true
+        if let doesServerAllowRetryString = doesServerAllowRetryString {
+            doesServerAllowRetry = Bool(doesServerAllowRetryString.lowercased()) ?? true


I'm sure this version is more clever, but it was kinda clearer before the ternary IMO

I'm okay reverting it! I'll keep the name doesServerAllowRetry over isRetryable though 😅

aboedo · 2024-07-24T20:33:17Z

Sources/Networking/HTTPClient/HTTPRequest.swift

-
-    init(method: Method, path: HTTPRequest.Path, nonce: Data? = nil) {
-        self.init(method: method, requestPath: path, nonce: nonce)
+    /// Whether or not this reqeust should be retried by the HTTPClient for certain status codes.


typo request

aboedo · 2024-07-24T20:35:55Z

Tests/BackendIntegrationTests/OtherIntegrationTests.swift

@@ -173,7 +175,7 @@ class OtherIntegrationTests: BaseBackendIntegrationTests {
        try AvailabilityChecks.iOS15APIAvailableOrSkipTest()

        let result = try await self.purchases.productEntitlementMapping().entitlementsByProduct
-        expect(result).to(haveCount(18))
+        expect(result).to(haveCount(21))


@RevenueCat/coresdk thoughts?

aboedo · 2024-07-24T20:37:05Z

Tests/BackendIntegrationTests/StoreKitIntegrationTests.swift

@@ -935,6 +935,7 @@ class StoreKit1IntegrationTests: BaseStoreKitIntegrationTests {
    }

    func testVerifyPurchaseGrantsEntitlementsThroughOnRetryAfter429() async throws {
+        defer { HTTPStubs.removeAllStubs() }


should this just be a setup or teardown?

Moved it to the tearDown functions 😎

fire-at-will · 2024-07-25T12:22:32Z

Hey @aboedo! Since your last review, I've made the following changes:

Code Changes

Reverted the syntax when computing doesServerAllowRetry
Fixed the "request" typo in HTTPRequest.swift
Moved the HTTPStubs.removeAllStubs() calls in the tests from a defer statements to the tearDown functions

Test Fixes/Clarifications

Fixed the configuration issues that were causing the CustomEntitlementsComputationIntegrationTests to fail
Dug into why OtherIntegrationTests.testProductEntitlementMapping was returning 21 mappings instead of 18. Josh and I took a look at some network logs and found that the backend was returning 18 mappings on Tuesday when the test passed, but is now returning 21 in the network calls to /v1/product_entitlement_mapping. This is happening on this branch, main, and the 4.32.2 release. We now believe that 21 is the correct number and are following up with the backend team to investigate what caused the change to take so long to appear.

This should be ready for another look!

only retry post receipt

bffe86f

fire-at-will added the refactor label Jul 24, 2024

fire-at-will added 3 commits July 24, 2024 11:27

linting

0a67843

distinguish path based on network operation

d00696b

make guard ordering clearer

0934f34

fire-at-will commented Jul 24, 2024

View reviewed changes

add HTTPRequest unit tests

cc26b70

fire-at-will requested review from aboedo, MarkVillacampa and a team July 24, 2024 17:34

fix tests

cf5eb84

fire-at-will commented Jul 24, 2024

View reviewed changes

linting

9b32c52

aboedo reviewed Jul 24, 2024

View reviewed changes

address PR feedback

43168dd

fire-at-will requested a review from aboedo July 25, 2024 12:22

aboedo approved these changes Jul 25, 2024

View reviewed changes

fire-at-will merged commit 348258a into main Jul 25, 2024
32 checks passed

fire-at-will deleted the only-retry-post-receipt branch July 25, 2024 21:46

fire-at-will mentioned this pull request Jul 25, 2024

Release/5.2.2 #4115

Merged

vegaro added pr:other and removed pr:refactor labels Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only Retry POST Receipt Paths for 429 #4107

Only Retry POST Receipt Paths for 429 #4107

fire-at-will commented Jul 24, 2024 •

edited

Loading

fire-at-will Jul 24, 2024

fire-at-will commented Jul 24, 2024

fire-at-will commented Jul 24, 2024

fire-at-will Jul 24, 2024

fire-at-will Jul 24, 2024

fire-at-will Jul 24, 2024

fire-at-will Jul 24, 2024

aboedo Jul 24, 2024

fire-at-will Jul 25, 2024

aboedo left a comment

aboedo Jul 24, 2024

fire-at-will Jul 24, 2024

aboedo Jul 24, 2024

aboedo Jul 24, 2024

aboedo Jul 24, 2024

fire-at-will Jul 25, 2024

fire-at-will commented Jul 25, 2024 •

edited

Loading

Only Retry POST Receipt Paths for 429 #4107

Only Retry POST Receipt Paths for 429 #4107

Conversation

fire-at-will commented Jul 24, 2024 • edited Loading

Description

Testing

Choose a reason for hiding this comment

fire-at-will commented Jul 24, 2024

fire-at-will commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fire-at-will commented Jul 25, 2024 • edited Loading

Code Changes

Test Fixes/Clarifications

fire-at-will commented Jul 24, 2024 •

edited

Loading

fire-at-will commented Jul 25, 2024 •

edited

Loading