Improves retries backoff for network errors (#151)

CHANGELOG.md

@@ -1,6 +1,6 @@
 # Change Log
 
-## [2.1.9] - 2024-06-26
+## [2.2.0] - 2024-07-09
 ### Changed
 - Fixed infinite retries for network errors
 

README.md

@@ -40,7 +40,7 @@ This will create initial files and folders including hidden​ `​.gitignore​
         dependencies: [
             // Dependencies declare other packages that this package depends on.
             ​// .package(url: /* package url */, from: "1.0.0"),
-            .​package​(​url​: "https://github.com/SiftScience/sift-ios.git"​, ​from​: "2.1.9"​)
+            .​package​(​url​: "https://github.com/SiftScience/sift-ios.git"​, ​from​: "2.2.0"​)
         ],
         ​targets​: [
             ​// Targets are the basic building blocks of a package. A target can define a module or a test suite.

Sift.podspec

@@ -1,6 +1,6 @@
 Pod::Spec.new do |spec|
   spec.name = 'Sift'
-  spec.version = '2.1.9'
+  spec.version = '2.2.0'
   spec.authors = 'Sift Science'
   spec.license = {
     :type => 'MIT',

Sift/Sift.m

@@ -57,7 +57,7 @@ + (instancetype)sharedInstance {
 - (instancetype)initWithRootDirPath:(NSString *)rootDirPath {
     self = [super init];
     if (self) {
-        _sdkVersion = @"v2.1.9";
+        _sdkVersion = @"v2.2.0";
 
         _rootDirPath = rootDirPath;
 

Sift/SiftUploader.h

@@ -10,7 +10,7 @@ NS_EXTENSION_UNAVAILABLE_IOS("SiftUploader is not supported for iOS extensions."
 - (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift;
 
 /** For testing. */
-- (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift config:(NSURLSessionConfiguration *)config backoffBase:(int64_t)backoffBase;
+- (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift config:(NSURLSessionConfiguration *)config backoffBase:(int64_t)backoffBase networkRetryTimeout:(int64_t)networkRetryTimeout;
 
 /** Persist uploader state to disk. */
 - (void)archive;

Sift/SiftUploader.m

@@ -21,6 +21,8 @@ @implementation SiftUploader {
     NSMutableData *_responseBody;
     NSMutableArray *_batches;
     int _numRejects;
+    int _numNetworkRejects;
+    int64_t _maxNumNetworkRejects;
     int64_t _backoffBase;
     int64_t _backoff;
     NSString *_archivePath;
@@ -31,17 +33,24 @@ @implementation SiftUploader {
 // Drop a batch if our backend has rejected it `SF_REJECT_LIMIT` times.
 static const int SF_REJECT_LIMIT = 3;
 
+// Batch will be dropped if the network requests fail and the retry timeout period is reached.
+static const int64_t SF_DEFAULT_NETWORK_RETRY_TIMEOUT = 10 * 60 * NSEC_PER_SEC;
+static const int64_t SF_FALLBACK_NETWORK_MAX_RETRIES = 60;
+
 static const int64_t SF_BACKOFF = NSEC_PER_SEC * 5;  // Starting from 5 seconds.
 
 // Periodically check if we have unfinished batches.
 static const int64_t SF_CHECK_UPLOAD_PERIOD = 60 * NSEC_PER_SEC;
 static const int64_t SF_CHECK_UPLOAD_LEEWAY = 5 * NSEC_PER_SEC;
 
 - (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift {
-    return [self initWithArchivePath:archivePath sift:sift config:[NSURLSessionConfiguration defaultSessionConfiguration] backoffBase:SF_BACKOFF];
+    return [self initWithArchivePath:archivePath sift:sift config:[NSURLSessionConfiguration defaultSessionConfiguration] backoffBase:SF_BACKOFF
+        networkRetryTimeout:SF_DEFAULT_NETWORK_RETRY_TIMEOUT];
 }
 
-- (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift config:(NSURLSessionConfiguration *)config backoffBase:(int64_t)backoffBase {
+- (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift config:(NSURLSessionConfiguration *)config backoffBase:(int64_t)backoffBase
+    networkRetryTimeout:(int64_t)networkRetryTimeout {
+
     self = [super init];
     if (self) {
         _taskManager = [[TaskManager alloc] init];
@@ -52,7 +61,10 @@ - (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift co
         _backoffBase = backoffBase;
         _backoff = backoffBase;
         _sift = sift;
-
+        _maxNumNetworkRejects = networkRetryTimeout / backoffBase;
+        if (_maxNumNetworkRejects <= 0) {
+            _maxNumNetworkRejects = SF_FALLBACK_NETWORK_MAX_RETRIES;
+        }
         [self unarchive];
 
         _source = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, _serial);
@@ -96,10 +108,13 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
         self->_responseBody = nil;
 
         BOOL success = NO;
+        BOOL networkError = NO;
         if (error) {
             SF_IMPORTANT(@"Could not complete upload due to %@", [error localizedDescription]);
-            self->_numRejects++;
+            self->_numNetworkRejects++;
+            networkError = YES;
         } else {
+            self->_numNetworkRejects = 0;
             NSInteger statusCode = [(NSHTTPURLResponse *)task.response statusCode];
             SF_DEBUG(@"PUT %@ status %ld", task.response.URL, (long)statusCode);
             if (statusCode != 200) {
@@ -123,26 +138,34 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
             }
         }
 
-        if (self->_numRejects >= SF_REJECT_LIMIT) {
+        if (self->_numRejects >= SF_REJECT_LIMIT ||
+            self->_numNetworkRejects >= self->_maxNumNetworkRejects) {
             NSLog(@"Drop a batch due to reject limit reached");
             [self->_batches removeObjectAtIndex:0];
             self->_numRejects = 0;
+            self->_numNetworkRejects = 0;
             self->_backoff = self->_backoffBase;
         }
 
         // Keep working on unfinished upload jobs.
         if (success) {
             self->_backoff = self->_backoffBase;
             [self doUpload];
+        } else if (networkError) {
+            [self retryUploadWithDelay:self->_backoffBase];
         } else {
-            [self->_taskManager scheduleWithTask:^{
-                [self doUpload];
-            } queue:self->_serial delay:self->_backoff];
+            [self retryUploadWithDelay:self->_backoff];
             self->_backoff *= 2;
         }
     } queue:self->_serial];
 }
 
+- (void)retryUploadWithDelay:(NSTimeInterval)delay {
+    [self->_taskManager scheduleWithTask:^{
+        [self doUpload];
+    } queue:self->_serial delay:delay];
+}
+
 - (void)doUpload {
     // Query the applicationState on the main thread, then proceed on the serial dispatch queue
     [self->_taskManager submitWithTask:^{

SiftTests/SiftUploaderNetworkBackoffTests.m

@@ -0,0 +1,74 @@
+//  Copyright © 2024 Sift Science. All rights reserved.
+
+@import XCTest;
+
+#import "SiftEvent.h"
+#import "Sift.h"
+#import "Sift+Private.h"
+
+#import "SiftUploader.h"
+
+#import "SiftStubHttpProtocol.h"
+
+@interface SiftUploaderBackoffTests : XCTestCase
+
+@end
+
+@implementation SiftUploaderBackoffTests {
+    Sift *_sift;
+    SiftUploader *_uploader;
+}
+
+- (void)setUp {
+    [super setUp];
+
+    _sift = [[Sift alloc] init];  // Don't call initWithRootDirPath.
+    _sift.accountId = @"account_id";
+    _sift.beaconKey = @"beacon_key";
+    _sift.userId = @"user_id";
+    _sift.serverUrlFormat = @"mock+https://127.0.0.1/v3/accounts/%@/mobile_events";
+
+    // Set backoff = 5 seconds to test calculation of max retries for network errors
+    _uploader = [[SiftUploader alloc] initWithArchivePath:nil sift:_sift config:SFMakeStubConfig() backoffBase:5 * NSEC_PER_SEC networkRetryTimeout: 60 * NSEC_PER_SEC];
+
+    SFHttpStub *stub = [SFHttpStub sharedInstance];
+    [stub.stubbedStatusCodes removeAllObjects];
+    [stub.capturedRequests removeAllObjects];
+}
+
+- (void)tearDown {
+    [super tearDown];
+}
+
+- (void)testUploadMaxRetriesForNetworkErrorsWithBaseBackoff {
+    SFHttpStub *stub = [SFHttpStub sharedInstance];
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for upload tasks completion"];
+    stub.completionHandler = ^{
+        [expectation fulfill];
+    };
+
+    // add stubs to the queue
+    int rejectedLimitCount = 100;
+    for (int i = 0; i < rejectedLimitCount; i++) {
+        // Simulate a network error response (status code 1 hardcoded for network errors).
+        [stub.stubbedStatusCodes addObject:@1];
+    }
+
+    NSArray *events = @[[SiftEvent eventWithType:nil path:@"path" fields:nil]];
+    [_uploader upload:events];
+
+    XCTWaiter *waiter = [[XCTWaiter alloc] init];
+    // max network retries should not exceed 1 min
+    XCTWaiterResult result = [waiter waitForExpectations:@[expectation] timeout:61.0];
+
+    // The waiter should time out because it's waiting for the final call (rejectedLimitCount = 100) to be made.
+    // However, the call is not made because the system stops retrying after max retries for network failed attempts (60 / backoffBase).
+    XCTAssertEqual(result, XCTWaiterResultTimedOut);
+    // Assert that no more than 12 (60 / backoffBase) calls were made
+    XCTAssertEqual(stub.capturedRequests.count, 60 / 5);
+    // 88 (initial stubs in the queue minus 12 captured requests) stubs should still be in the queue
+    XCTAssertEqual(stub.stubbedStatusCodes.count, rejectedLimitCount - 60/5);
+}
+
+@end

SiftTests/SiftUploaderTests.m

@@ -29,7 +29,7 @@ - (void)setUp {
     _sift.serverUrlFormat = @"mock+https://127.0.0.1/v3/accounts/%@/mobile_events";
 
     // Disable exponential backoff with baseoffBase = 0.
-    _uploader = [[SiftUploader alloc] initWithArchivePath:nil sift:_sift config:SFMakeStubConfig() backoffBase:0];
+    _uploader = [[SiftUploader alloc] initWithArchivePath:nil sift:_sift config:SFMakeStubConfig() backoffBase:0 networkRetryTimeout: 60 * NSEC_PER_SEC];
 
     SFHttpStub *stub = [SFHttpStub sharedInstance];
     [stub.stubbedStatusCodes removeAllObjects];
@@ -107,7 +107,7 @@ - (void)testUploadWithNetworkError {
         [expectation fulfill];
     };
 
-    int rejectedLimitCount = 3;
+    int rejectedLimitCount = 60;
     for (int i = 0; i < rejectedLimitCount; i++) {
         // Simulate a network error response (status code 1 hardcoded for network errors).
         [stub.stubbedStatusCodes addObject:@1];
@@ -130,7 +130,7 @@ - (void)testUploadWithNetworkErrorAndTimeout {
         [expectation fulfill];
     };
 
-    int rejectedLimitCount = 4;
+    int rejectedLimitCount = 61;
     for (int i = 0; i < rejectedLimitCount; i++) {
         // Simulate a network error response (status code 1 hardcoded for network errors).
         [stub.stubbedStatusCodes addObject:@1];
@@ -142,13 +142,76 @@ - (void)testUploadWithNetworkErrorAndTimeout {
     XCTWaiter *waiter = [[XCTWaiter alloc] init];
     XCTWaiterResult result = [waiter waitForExpectations:@[expectation] timeout:3.0];
 
-    // The waiter should time out because it's waiting for the final call (call 4) to be made.
-    // However, the call is not made because the system stops retrying after three failed attempts (SF_REJECT_LIMIT).
+    // The waiter should time out because it's waiting for the final call (call 61) to be made.
+    // However, the call is not made because the system stops retrying after 60 failed attempts (SF_DEFAULT_NETWORK_MAX_RETRIES).
     XCTAssertEqual(result, XCTWaiterResultTimedOut);
-    // Assert that no more than 3(SF_REJECT_LIMIT) calls were made
-    XCTAssertEqual(stub.capturedRequests.count, 3);
+    // Assert that no more than 60(SF_DEFAULT_NETWORK_MAX_RETRIES) calls were made
+    XCTAssertEqual(stub.capturedRequests.count, 60);
     // 1 stub should still be in the queue
     XCTAssertEqual(stub.stubbedStatusCodes.count, 1);
 }
 
+- (void)testUploadNetworkErrorsMaxRetriesNotReached {
+    SFHttpStub *stub = [SFHttpStub sharedInstance];
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for upload tasks completion"];
+    stub.completionHandler = ^{
+        [expectation fulfill];
+    };
+
+    int rejectedLimitCount = 59;
+    for (int i = 0; i < rejectedLimitCount; i++) {
+        // Simulate a network error response (status code 1 hardcoded for network errors).
+        [stub.stubbedStatusCodes addObject:@1];
+    }
+
+    int successfullRequests = 1;
+    for (int i = 0; i < successfullRequests; i++) {
+        [stub.stubbedStatusCodes addObject:@200];
+    }
+
+    NSArray *events = @[[SiftEvent eventWithType:nil path:@"path" fields:nil]];
+    [_uploader upload:events];
+
+    [self waitForExpectationsWithTimeout:3.0 handler:nil];
+
+    XCTAssertEqual(stub.capturedRequests.count, rejectedLimitCount + successfullRequests);
+    XCTAssertEqual(stub.stubbedStatusCodes.count, 0);
+}
+
+- (void)testUploadNetworkAndHttpErrorsMaxRetriesNotReached {
+    SFHttpStub *stub = [SFHttpStub sharedInstance];
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for upload tasks completion"];
+    stub.completionHandler = ^{
+        [expectation fulfill];
+    };
+
+    int rejectedLimitCount = 59;
+    for (int i = 0; i < rejectedLimitCount; i++) {
+        // Simulate a network error response (status code 1 hardcoded for network errors).
+        [stub.stubbedStatusCodes addObject:@1];
+    }
+
+    int rejectedHttpErrorLimitCount = 2;
+    for (int i = 0; i < rejectedHttpErrorLimitCount; i++) {
+        [stub.stubbedStatusCodes addObject:@500];
+    }
+
+    int successfullRequests = 1;
+    for (int i = 0; i < successfullRequests; i++) {
+        [stub.stubbedStatusCodes addObject:@200];
+    }
+
+    NSArray *events = @[[SiftEvent eventWithType:nil path:@"path" fields:nil]];
+    [_uploader upload:events];
+
+    [self waitForExpectationsWithTimeout:3.0 handler:nil];
+
+
+    XCTAssertEqual(stub.capturedRequests.count, rejectedLimitCount + successfullRequests + rejectedHttpErrorLimitCount);
+    // verify that max retries for network and http errors are not reached
+    XCTAssertEqual(stub.stubbedStatusCodes.count, 0);
+}
+
 @end