fix(DataStore): retry initial sync network failures from RemoteSyncEngine #1773

lawmicha · 2022-05-03T14:07:21Z

Description:

This PR fixes RemoteSyncEngine to accurately determine whether it should retry based on the URLError. Previously, when errors were handled, they were ignored and a hardcoded URLError.notConnected is passed to the retryRequestAdvice method to get the advice to retry. This is a bug when the error is a non-retryable error, since the RemoteSyncEngine's retry attempt will immediately fall into the same errored state. For example, data decoding issues which should be non-retryable but the bug caused the sync engine to restart a number of times up to the limit determine by the algorithm in RequestRetryablePolicy.

Changes made

RemoteSyncEngine's error handling path to stop using the hardcoded URLError and start checking against a real URLError, retrieved from the underlying error. All components, specifically the sync and subscription components, are responsible for populating the underlying error that it receives from Amplify.API.
AWSInitialSyncOrchestrator: If any of the syncQuery requests fail with a network error, pick the first one to be the underlying error. This is returned as a DataStoreError.sync error with the URLError as the underlying error.

Testing Initial Sync - AWSInitialSyncOrchestrator

An end-to-end flow is when the component propagates the URLError back up to RemoteSyncEngine and RemoteSyncEngine accurately determines that it should retry.

network on, DataStore.start(), then breakpoint into InitialSyncOperation's query(lastSyncTime:, nextToken:)
network off, See that API returns a failure and AWSInitialSyncOrchestrator pulls it out and into the DataStoreError.sync error.

Testing data decoding issue

A schema with a model containing enum cases like

type Todo @model {
  id: ID!
  name: String!
  description: String
  status: Status!
}

enum Status {
  notStarted
  inProgress
  completed
  blocked
}

save some data, and then update the Status enum to remove one of the cases

public enum Status: String, EnumPersistable {
  case notStarted
  case inProgress
  case completed
  //case blocked
}

DataStore.start() should sync the data and receive the error

dataCorrupted(Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "items", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "status", intValue: nil)], debugDescription: "Cannot initialize Status from invalid String value blocked", underlyingError: nil))

Observe that no retries are made.

Subscriptions

Amplify.API.subscription API will use the AppSyncRealTimeClient library for subscriptions. When there is a network status changes, it will internally disconnect and reconnect with exponential back-ff. From Amplify.API's perspective, it doesn't receive an error until all retry attempts have been exhausted, and the error is an .operationError, not a URLError. See aws-amplify/aws-appsync-realtime-client-ios#58 for more details

Sync to Cloud Mutations

OutgoingMutationQueue will create a SyncMutationtoCloudOperation for the mutation event, which contains retry logic for network failures. See #1722 for more details

Check points: (check or cross out if not relevant)

Added new tests to cover change, if needed
Build succeeds with all target using Swift Package Manager
All unit tests pass
All integration tests pass
Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
Documentation update for the change if required
PR title conforms to conventional commit style
If breaking change, documentation/changelog update with migration instructions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…gine

diegocstn · 2022-05-03T16:37:06Z

...yPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOrchestrator.swift

@@ -158,10 +158,16 @@ final class AWSInitialSyncOrchestrator: InitialSyncOrchestrator {
            return .successfulVoid
        }

+        var underlyingError: Error?
+        if syncErrors.contains(where: isNetworkError) {
+            underlyingError = getFirstUnderlyingNetworkError(errors: syncErrors)


Any potential side effects on using the first network error? (ie. can different network errors have different retry policies?)

nit: can we use .first(where predicate:) and check if syncErrors has any network error and grab the first in a single operation?

Currently, it is not ideal to have RemoteSyncEngine contain URLError based retry logic, and this is why it's forcing us to propagate one of the network errors up to the RemoteSyncEngine. Ideally, InitialSyncOperation should be responsible for checking the network error, determining that the error is retryable, and retry with the exponential back-off. This means it will cycle on the single InitialSyncOperation trying to sync that one model. Once the network is back up, then the operation will finish, and move onto the next InitialSyncOperation.

This code current assumes that the first network error is probably similar to other network errors if they were run approximately the same time. Of course this assumption could be wrong, but the side effect of it being wrong and pick up a network error that RemoteSyncEngine decides is non-retryable is that the sync engine will move to an errored state, as with all other non-retryable errors.

The retry policy i believe is determined by the implementation in RequestRetryablePolicy which filters down to

case .notConnectedToInternet, .dnsLookupFailed, .cannotConnectToHost, .cannotFindHost, .timedOut:

thisisabhash · 2022-05-03T16:44:27Z

...yPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOrchestrator.swift

@@ -219,4 +225,39 @@ extension AWSInitialSyncOrchestrator {
        }
        return false
    }
+
+    private func isNetworkError(_ error: DataStoreError) -> Bool {
+        guard case let .sync(_, _, underlyingError) = error,


is this check required if we are only looking for network error?

This is required because the InitialSyncOrchestrator wraps the error from the operations in a DataStoreError.sync at line 120, stores in syncErrors, and moves onto the next operation. Comes to think of it, i'm not really sure why it needs to be this way, since syncErrors is just an array of [DataStoreError]. But as it is currently, it needs to unwrap the .sync case like it's done above in isUnauthorizedError

if case .failure(let dataStoreError) = result { let syncError = DataStoreError.sync( "An error occurred syncing \(modelSchema.name)", "", dataStoreError) self.syncErrors.append(syncError)

thisisabhash · 2022-05-03T16:44:43Z

...yPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOrchestrator.swift

+
+    private func getFirstUnderlyingNetworkError(errors: [DataStoreError]) -> Error? {
+        for error in errors {
+            guard case let .sync(_, _, underlyingError) = error,


same comment as previous

codecov-commenter · 2022-05-04T04:21:17Z

Codecov Report

Merging #1773 (a8fb911) into main (8f635d0) will decrease coverage by 0.05%.
The diff coverage is 18.75%.

@@            Coverage Diff             @@
##             main    #1773      +/-   ##
==========================================
- Coverage   59.23%   59.18%   -0.06%     
==========================================
  Files         716      716              
  Lines       21916    21943      +27     
==========================================
+ Hits        12982    12987       +5     
- Misses       8934     8956      +22

Flag	Coverage Δ
API_plugin_unit_test	`62.51% <ø> (ø)`
AWSPluginsCore	`71.37% <ø> (+0.07%)`	⬆️
Amplify	`46.83% <ø> (+0.01%)`	⬆️
Analytics_plugin_unit_test	`71.09% <ø> (ø)`
Auth_plugin_unit_test	`78.74% <ø> (ø)`
DataStore_plugin_unit_test	`75.80% <18.75%> (-0.31%)`	⬇️
Geo_plugin_unit_test	`52.12% <ø> (ø)`
Predictions_plugin_unit_test	`26.81% <ø> (ø)`
Storage_plugin_unit_test	`58.04% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...gin/Sync/InitialSync/InitialSyncOrchestrator.swift	`50.76% <0.00%> (-11.50%)`	⬇️
...tegoryPlugin/Sync/RemoteSyncEngine+Retryable.swift	`77.50% <85.71%> (-0.88%)`	⬇️
...gin/Sync/InitialSync/ModelSyncedEventEmitter.swift	`89.58% <0.00%> (-1.05%)`	⬇️
...ataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift	`96.00% <0.00%> (+0.79%)`	⬆️
.../DataStore/Model/Internal/Schema/ModelSchema.swift	`66.00% <0.00%> (+2.00%)`	⬆️
...e/AWSPluginsCore/Auth/AWSAuthServiceBehavior.swift	`87.50% <0.00%> (+12.50%)`	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

diegocstn · 2022-05-06T19:50:20Z

...yPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOrchestrator.swift

-        if syncErrors.contains(where: isNetworkError) {
-            underlyingError = getFirstUnderlyingNetworkError(errors: syncErrors)
+        if let error = syncErrors.first(where: isNetworkError(_:)) {
+            underlyingError = getFirstUnderlyingNetworkError(error)


nit: getFirstUnderlyingNetworkError -> getUnderlyingNetworkError

lawmicha requested a review from a team May 3, 2022 14:07

fix(DataStore): retry initial sync network failures from RemoteSyncEn…

9161e3f

…gine

lawmicha force-pushed the fix/retry-initial-sync branch from a230ac3 to 9161e3f Compare May 3, 2022 14:08

diegocstn reviewed May 3, 2022

View reviewed changes

thisisabhash reviewed May 3, 2022

View reviewed changes

lawmicha added 2 commits May 3, 2022 16:39

address PR comments

8a5d452

add functional test

79532fe

lawmicha force-pushed the fix/retry-initial-sync branch from 2e581ac to 79532fe Compare May 4, 2022 04:00

lawmicha self-assigned this May 4, 2022

thisisabhash approved these changes May 5, 2022

View reviewed changes

diegocstn reviewed May 6, 2022

View reviewed changes

diegocstn approved these changes May 6, 2022

View reviewed changes

address PR comment

a8fb911

diegocstn approved these changes May 6, 2022

View reviewed changes

lawmicha merged commit 1961d61 into main May 8, 2022

lawmicha deleted the fix/retry-initial-sync branch May 8, 2022 15:08

lawmicha mentioned this pull request May 27, 2022

Initial sync missing __typename and going to endless cycle. #1458

Closed

anatomybook mentioned this pull request Jun 11, 2022

DataStore does not recover after extended background time #1839

Closed

lawmicha mentioned this pull request Jun 14, 2022

fix(datastore): stop sync engine on non-retryable errors to allow restart #1901

Merged

8 tasks

atierian mentioned this pull request Jun 30, 2023

SyncEngine should inspect errors to determine retry advice #731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(DataStore): retry initial sync network failures from RemoteSyncEngine #1773

fix(DataStore): retry initial sync network failures from RemoteSyncEngine #1773

lawmicha commented May 3, 2022

diegocstn May 3, 2022

diegocstn May 3, 2022

lawmicha May 3, 2022

thisisabhash May 3, 2022

lawmicha May 3, 2022

thisisabhash May 3, 2022

codecov-commenter commented May 4, 2022 •

edited

Loading

diegocstn May 6, 2022

fix(DataStore): retry initial sync network failures from RemoteSyncEngine #1773

fix(DataStore): retry initial sync network failures from RemoteSyncEngine #1773

Conversation

lawmicha commented May 3, 2022

Description:

Changes made

Testing Initial Sync - AWSInitialSyncOrchestrator

Testing data decoding issue

Subscriptions

Sync to Cloud Mutations

diegocstn May 3, 2022

Choose a reason for hiding this comment

diegocstn May 3, 2022

Choose a reason for hiding this comment

lawmicha May 3, 2022

Choose a reason for hiding this comment

thisisabhash May 3, 2022

Choose a reason for hiding this comment

lawmicha May 3, 2022

Choose a reason for hiding this comment

thisisabhash May 3, 2022

Choose a reason for hiding this comment

codecov-commenter commented May 4, 2022 • edited Loading

Codecov Report

diegocstn May 6, 2022

Choose a reason for hiding this comment

codecov-commenter commented May 4, 2022 •

edited

Loading