[MBL-19602][S/T/P] Revision of API rate limit exceeded retrial logic (#3809)

refs: MBL-19602
affects: Student, Teacher, Parent
builds: Student, Teacher, Parent
release note: none

Author: suhaibabsi-inst

Core/Core/Common/CommonModels/API/API.swift

@@ -60,8 +60,8 @@ public class API {
                 }
 
                 // If the request is rejected due to the rate limit being exhausted we retry and hope that the quota is restored in the meantime
-                if response?.exceededLimit(responseData: data) == true {
-                    DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(1)) { [weak self] in
+                if let retrialTime = response?.retrialTimeOnRateLimitExceeded(responseData: data) {
+                    DispatchQueue.main.asyncAfter(deadline: retrialTime) { [weak self] in
                         self?.makeRequest(requestable, callback: callback)
                     }
                     return

Core/Core/Common/Extensions/Foundation/URLResponseExtensions.swift

@@ -59,7 +59,18 @@ extension URLResponse {
         }
 
         // hasPrefix is because we don't care about the line break character at the end
-        return httpResponse.statusCode == 403 && stringData.hasPrefix("403 Forbidden (Rate Limit Exceeded)")
+        let is403 = httpResponse.statusCode == 403 && stringData.hasPrefix("403 Forbidden (Rate Limit Exceeded)")
+
+        // Generally, received as "429 Too Many Requests (Rate Limit Exceeded)"
+        let is429 = httpResponse.statusCode == 429 && stringData.contains("(Rate Limit Exceeded)")
+
+        return is403 || is429
+    }
+
+    public func retrialTimeOnRateLimitExceeded(responseData: Data?) -> DispatchTime? {
+        guard exceededLimit(responseData: responseData) else { return nil }
+
+        return .now() + .seconds(1) + .milliseconds(Int.random(in: 0...500))
     }
 }
 

Core/CoreTests/Common/CommonModels/API/APITests.swift

@@ -82,6 +82,7 @@ class APITests: XCTestCase {
     override func setUp() {
         super.setUp()
         API.resetMocks(useMocks: false)
+        LoginSession.clearAll()
         AppEnvironment.shared.userDidLogin(session: .make())
     }
 
@@ -355,18 +356,26 @@ class APITests: XCTestCase {
         XCTAssertNotNil(error)
     }
 
-    func testRetryOnRateLimitedRequest() throws {
+    func testRetryOnRateLimitedRequest_403() throws {
+        testRetry(forErrorCode: 403, message: "403 Forbidden (Rate Limit Exceeded)\n")
+    }
+
+    func testRetryOnRateLimitedRequest_429() throws {
+        testRetry(forErrorCode: 429, message: "429 Too Many Requests (Rate Limit Exceeded)")
+    }
+
+    private func testRetry(forErrorCode code: Int, message: String) {
         API.resetMocks()
 
         let request = GetNoContent()
-        var invocationCount = 0
 
+        var invocationCount = 0
         api.mock(withData: request) { _ in
             switch invocationCount {
             case 0:
                 invocationCount += 1
-                let rateLimitResponse = HTTPURLResponse(url: .make(), statusCode: 403, httpVersion: nil, headerFields: nil)!
-                let rateLimitData = "403 Forbidden (Rate Limit Exceeded)\n".data(using: .utf8)!
+                let rateLimitResponse = HTTPURLResponse(url: .make(), statusCode: code, httpVersion: nil, headerFields: nil)!
+                let rateLimitData = message.data(using: .utf8)!
                 return (data: rateLimitData, response: rateLimitResponse, error: nil)
             case 1:
                 invocationCount += 1
@@ -377,6 +386,7 @@ class APITests: XCTestCase {
                 return (data: nil, response: nil, error: nil)
             }
         }
+
         let responseExpectation = expectation(description: "API response")
         var receivedResponse: URLResponse?
 

Core/CoreTests/Common/Extensions/Foundation/URLResponseExtensionsTests.swift

@@ -75,12 +75,54 @@ class URLResponseExtensionsTests: XCTestCase {
     }
 
     func testExceededLimit() {
-        let httpResponse = HTTPURLResponse(url: URL(string: "https://instructure.com")!, statusCode: 403, httpVersion: "HTTP/2.0", headerFields: [:])!
-        let data = "403 Forbidden (Rate Limit Exceeded)".data(using: .utf8)
+        var httpResponse = HTTPURLResponse(url: URL(string: "https://instructure.com")!, statusCode: 403, httpVersion: "HTTP/2.0", headerFields: [:])!
+        var data = "403 Forbidden (Rate Limit Exceeded)".data(using: .utf8)
+        XCTAssertTrue(httpResponse.exceededLimit(responseData: data))
 
+        httpResponse = HTTPURLResponse(url: URL(string: "https://instructure.com")!, statusCode: 429, httpVersion: "HTTP/2.0", headerFields: [:])!
+        data = "429 Too Many Requests (Rate Limit Exceeded)".data(using: .utf8)
         XCTAssertTrue(httpResponse.exceededLimit(responseData: data))
     }
 
+    func testRetrialTimeOnRateLimitExceeded() throws {
+        // Given
+        let httpResponse = HTTPURLResponse(url: URL(string: "https://instructure.com")!, statusCode: 429, httpVersion: "HTTP/2.0", headerFields: [:])!
+        let data = "429 Too Many Requests (Rate Limit Exceeded)".data(using: .utf8)
+
+        let base = DispatchTime.now()
+        let limit = base + .seconds(1) + .milliseconds(500)
+
+        // When
+        let retrialTimes = [
+            try XCTUnwrap(httpResponse.retrialTimeOnRateLimitExceeded(responseData: data)),
+            try XCTUnwrap(httpResponse.retrialTimeOnRateLimitExceeded(responseData: data)),
+            try XCTUnwrap(httpResponse.retrialTimeOnRateLimitExceeded(responseData: data)),
+            try XCTUnwrap(httpResponse.retrialTimeOnRateLimitExceeded(responseData: data))
+        ]
+
+        // Then
+        if case .nanoseconds(let interval) = base.distance(to: limit) {
+            for time in retrialTimes {
+                if case .nanoseconds(let nanoseconds) = base.distance(to: time) {
+                    XCTAssertTrue(nanoseconds <= interval)
+                } else {
+                    XCTFail("Time must be checked against limit")
+                }
+            }
+        } else {
+            XCTFail("All times must be checked against limit")
+        }
+
+        for i in 0 ..< retrialTimes.count {
+            let time = retrialTimes[i]
+
+            for j in 0 ..< retrialTimes.count where j != i {
+                let anotherTime = retrialTimes[j]
+                XCTAssertNotEqual(time, anotherTime)
+            }
+        }
+    }
+
     func testExceededLimitWithoutData() {
         let httpResponse = HTTPURLResponse(url: URL(string: "https://instructure.com")!, statusCode: 403, httpVersion: "HTTP/2.0", headerFields: [:])!
 
@@ -89,15 +131,21 @@ class URLResponseExtensionsTests: XCTestCase {
 
     func testExceededLimitWithMismatchingData() {
         let httpResponse = HTTPURLResponse(url: URL(string: "https://instructure.com")!, statusCode: 403, httpVersion: "HTTP/2.0", headerFields: [:])!
-        let data = "403 Forbidden".data(using: .utf8)
+        var data = "403 Forbidden".data(using: .utf8)
 
         XCTAssertFalse(httpResponse.exceededLimit(responseData: data))
+
+        data = "429 Too Many Requests".data(using: .utf8)
+        XCTAssertFalse(httpResponse.exceededLimit(responseData: data))
     }
 
     func testExceededLimitWithNonHTTPResponse() {
         let response = URLResponse(url: URL(string: "https://instructure.com")!, mimeType: nil, expectedContentLength: 0, textEncodingName: nil)
-        let data = "403 Forbidden (Rate Limit Exceeded)".data(using: .utf8)
 
+        var data = "403 Forbidden (Rate Limit Exceeded)".data(using: .utf8)
+        XCTAssertFalse(response.exceededLimit(responseData: data))
+
+        data = "429 Too Many Requests (Rate Limit Exceeded)".data(using: .utf8)
         XCTAssertFalse(response.exceededLimit(responseData: data))
     }
 }