Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(DataStore): improve MutationEvent resiliency to interruptions #3492

Merged
merged 8 commits into from
Feb 20, 2024

Conversation

lawmicha
Copy link
Member

@lawmicha lawmicha commented Feb 1, 2024

Issue #

Continuation from #3259 (comment)

This PR is a two distinct changes that together improves the resiliency of interruptions caused by DataStore.stop.

  1. Ensure MutationEvents are persisted, or otherwise the operation fails (DataStore.save)
  2. Ensure that OutgoingMutationQueue always processes the latest MutationEvent, regardless of InProcess state.

1. save and syncMutation under transaction

When DataStore.save() is called, it will perform a sequence of reads and writes. If this sequence is interrupted by DataStore.stop() then not all data that would have been written is committed. For example, SQLiteStorageEngineAdapter.save() will check if the model exists, decide whether to create an Insert/update, perform the write, and query the model and return it. Then, StorageEngine.save() checks if sync is enabled, and saves the MutationEvent through syncMutation(of: savedModel). On completion, the results is returned to the caller of DataStore.save(). Ref: #3259 (comment)

This PR aims to solve this: If the process is interrupted before the MutationEvent is persisted, then the local data will never be synced to AppSync.

The problem causes subsequent issues for models with associations, ie. Comment belongs to a Post. If the post is never synced to AppSync, then saving the comment will result in

  1. Save comment to local database is fine, since there's a local post
  2. Sync comment to AppSync fails when the schema enforces required association. Post does not exist in AppSync.

The changes in the PR to add the operations under a transaction guarantees that the two writes are done atomically, or otherwise DataStore.save will fail. This doesn't allow the local database to be put in a state where the model is committed but the mutation event is not. In the above example, the post in the model tabel and the mutation event table will only exist if both were successful. If we save the comment without the post, then the local DB operation should fail because there is no parent post

2. MutationEvent dequeue include inProcess true events

It can be observed in highly concurrent calls to DataStore.start, DataStore.stop, DataStore.save that it will interrupt the OutgoingMutationQueue process of processing a MutationEvent and deleting it from the queue (removing it from local storage). DataStore.start, which starts the remote sync engine, should perform a step to move all inProcess MutationEvents back to false, however there's a timing issue that is difficult to pinpoint. OutgoingMutationQueue's query manages to pick up the second MutationEvent in the queue and sends it off, while the first one that is marked as inProcess isn't being processed, likely that process was already interrupted. Ref #3259 (comment)

This PR updates the logic to retrieve the head of the MutationEvent queue, instead of just the InProcess false events.

There is no concern over multiple processes picking up the same InProcess true event since OutgoingMutationQueue operates on one MutationEvent at a time. This flag is still needed for the handling consecutive update scenarios. We probably don't need the remote sync engine's start up step to move things back to InProcess false, but that optimization can be done outside of this PR.

General Checklist

  • 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
  • New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
  • 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.

@lawmicha lawmicha changed the title fix(DataStore): save and syncMutation under transaction fix(DataStore): save and syncMutation under transaction fix(DataStore): MutationEvent dequeue include inProcess true events Feb 5, 2024
@lawmicha lawmicha changed the title fix(DataStore): save and syncMutation under transaction fix(DataStore): MutationEvent dequeue include inProcess true events fix(DataStore): save and syncMutation under transaction, MutationEvent dequeue include inProcess true events Feb 5, 2024
@lawmicha lawmicha changed the title fix(DataStore): save and syncMutation under transaction, MutationEvent dequeue include inProcess true events fix(DataStore): improve MutationEvent resiliency to interruptions Feb 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (f64c471) 67.90% compared to head (8bef5fc) 68.18%.
Report is 23 commits behind head on main.

Files Patch % Lines
...n/Storage/SQLite/StorageEngineAdapter+SQLite.swift 77.41% 7 Missing ⚠️
...ces/AWSDataStorePlugin/Storage/StorageEngine.swift 92.85% 2 Missing ⚠️
...SMutationDatabaseAdapter+MutationEventSource.swift 95.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3492      +/-   ##
==========================================
+ Coverage   67.90%   68.18%   +0.27%     
==========================================
  Files        1080     1080              
  Lines       36406    36426      +20     
==========================================
+ Hits        24723    24838     +115     
+ Misses      11683    11588      -95     
Flag Coverage Δ
API_plugin_unit_test 67.23% <ø> (ø)
AWSPluginsCore 64.18% <ø> (ø)
Amplify 48.10% <ø> (ø)
Analytics_plugin_unit_test 81.16% <ø> (ø)
Auth_plugin_unit_test 78.98% <ø> (+0.02%) ⬆️
CoreMLPredictions_plugin_unit_test 59.44% <ø> (ø)
DataStore_plugin_unit_test 81.84% <87.34%> (+1.42%) ⬆️
Geo_plugin_unit_test 70.75% <ø> (ø)
Logging_plugin_unit_test 63.34% <ø> (ø)
Predictions_plugin_unit_test 37.16% <ø> (ø)
PushNotifications_plugin_unit_test 87.13% <ø> (ø)
Storage_plugin_unit_test 77.51% <ø> (ø)
unit_tests 68.18% <87.34%> (+0.27%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lawmicha lawmicha marked this pull request as ready for review February 14, 2024 20:48
@lawmicha lawmicha requested a review from a team as a code owner February 14, 2024 20:48
@lawmicha lawmicha force-pushed the lawmicha.save-sync-transaction branch from 86bb64e to 9fab7e9 Compare February 14, 2024 20:49
@lawmicha lawmicha merged commit f1d354c into main Feb 20, 2024
78 checks passed
@lawmicha lawmicha deleted the lawmicha.save-sync-transaction branch February 20, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants