-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
DataStore saves inconsistent data to dynamoDB on a poor connection #9979
Comments
@duckbytes We reviewed this yesterday and can reproduce, it looks to be a specific race condition as you've identified during in-flight network requests. In order to address this though it would be helpful to see the Cypress tests as well. Can you share those? |
Hey @undefobj. Thanks for looking into it. Cypress.Commands.add("addSingleTask", () => {
cy.get("[data-cy=NEW-title-skeleton]", { timeout: 10000 }).should(
"not.exist"
);
cy.get(".MuiPaper-root").should("be.visible");
cy.get("[data-cy=create-task-button]").click();
cy.get("[data-cy=save-to-dash-button]").click();
});
Cypress.Commands.add("loginByCognitoApi", (username, password) => {
const log = Cypress.log({
displayName: "COGNITO LOGIN",
message: [`🔐 Authenticating | ${username}`],
// @ts-ignore
autoEnd: false,
});
log.snapshot("before");
const signIn = Auth.signIn({ username, password });
cy.wrap(signIn, { log: false }).then((cognitoResponse) => {
const keyPrefixWithUsername = `${cognitoResponse.keyPrefix}.${cognitoResponse.username}`;
window.localStorage.setItem(
`${keyPrefixWithUsername}.idToken`,
cognitoResponse.signInUserSession.idToken.jwtToken
);
window.localStorage.setItem(
`${keyPrefixWithUsername}.accessToken`,
cognitoResponse.signInUserSession.accessToken.jwtToken
);
window.localStorage.setItem(
`${keyPrefixWithUsername}.refreshToken`,
cognitoResponse.signInUserSession.refreshToken.token
);
window.localStorage.setItem(
`${keyPrefixWithUsername}.clockDrift`,
cognitoResponse.signInUserSession.clockDrift
);
window.localStorage.setItem(
`${cognitoResponse.keyPrefix}.LastAuthUser`,
cognitoResponse.username
);
window.localStorage.setItem(
"amplify-authenticator-authState",
"signedIn"
);
log.snapshot("after");
log.end();
});
cy.visit("/");
});
describe("task actions", () => {
beforeEach(() => {
cy.loginByCognitoApi(Cypress.env("username"), Cypress.env("password"));
});
it("picked up, delivered, rider home", () => {
cy.visit("/");
cy.addSingleTask();
cy.get("[data-cy=tasks-kanban-column-NEW]").children().first().click();
cy.get("[data-cy=combo-box-riders]").click().type("Test Rider");
cy.get('[id*="option-0"]').should("exist");
cy.get('[id*="option-0"]').click();
cy.get("[data-cy=task-status]").should("have.text", "ACTIVE");
cy.get("[data-cy=task-RIDER-assignees]").contains("Test Rider");
cy.get("[data-cy=task-timePickedUp-button]").should("be.enabled");
cy.get("[data-cy=task-timePickedUp-button]").click();
cy.get("[data-cy=confirmation-ok-button]").click();
cy.get("[data-cy=task-status]").should("have.text", "PICKED UP");
cy.get("[data-cy=task-timeDroppedOff-button]").should("be.enabled");
cy.get("[data-cy=task-timeDroppedOff-button]").click();
cy.get("[data-cy=confirmation-ok-button]").click();
cy.get("[data-cy=task-status]").should("have.text", "DELIVERED");
cy.get("[data-cy=task-timeRiderHome-button]").should("be.enabled");
cy.get("[data-cy=task-timeRiderHome-button]").click();
cy.get("[data-cy=confirmation-ok-button]").click();
cy.get("[data-cy=task-status]").should("have.text", "COMPLETED");
});
}); This is the Cypress code. A lot of it applies to my app and not the example I made, but it might be helpful anyway. |
The Cypress test is interesting because in this case the network tab shows what looks like a correct mutation, but the observer returns the status in the top bar back to delivered: The test also passes because the UI does show completed for a moment before the observer catches up and puts it back to delivered. But the dynamodb record has the incorrect status again: This mismatch of recorded data seems to be reproducible for every run of the test. Edit to add: not sure if it's helpful too but these are the the unit tests that demonstrate exactly what I'm doing with DataStore underneath https://github.com/platelet-app/platelet/blob/master/src/scenes/Task/components/TaskActions.test.js |
@duckbytes thank you for this. We have determined the issue is due to a comparison problem between changed fields on the clients and the network response when using AutoMerge. We have a plan to solve this but it will take some care on our side for this scenario with testing. In the meantime if you are blocked on this particular behavior we would suggest switching your Conflict Resolution strategy to Optimistic Concurrency rather than AutoMerge if possible. |
Thanks @undefobj switching to optimistic concurrency looks to have fixed it. |
Hello, allow me to join this discussion to say that indeed we were facing a problem without understanding the cause. Eventually I stumbled across this thread and came to the same conclusion. We are developing a stock management application and unfortunately the inconsistencies that have arisen affect the satisfaction of our customers. Amplify is a wonderful tool and we appreciate the efforts made so far to resolve the issues we are having. We are still waiting for a definitive solution. Regards |
@frankarnaud thank you! We are currently focused on completing and releasing support for Custom Primary & Composite Keys in DataStore. However, the work for this fix is already up for review and will be scheduled for released afterwards. Please stay tuned for more updates! |
We too have an issue with DataStore thinking that all of the records have been synced to cloud (sending outboxStatus event with empty payload), but in reality these records are not persisted in DynamoDB. This often happens when we use our application in an area with poor network connectivity. Everything works when the app is completely online or completely offline. Things start breaking when the network connection varies in strength (e.g. good connection - bad connection - no connection - etc). Most of our users use the app in these network conditions. We tried following the suggestion in this thread and switched to Optimistic Concurrency conflict resolution strategy, but it seems to have little effect. Steps to reproduce: drive out of town until you get 3G cell signal with 0-2 bars. Create 100 entities in DataStore while walking in a line between two spots where signal strength varies. Return to the area with good Internet connection and observe up to 10% of the records missing in DynamoDB. Even after returning to good Internet connection, DataStore will not attempt to sync the remaining records as it thinks that there is nothing left to sync. |
Since this was a critical blocker for us, we ended up writing custom logic that listens for DataStore event that claims all records are synced. It then queries all model types and iterates over the results to find records with _version === undefined. For each such record, we submit a worker that calls AppSync API to create the record, then DataStore.query with that record ID predicate to force refresh it with the latest values. This is using AUTO_MERGE conflict resolution strategy. We are also using a token-based retry strategy in case we go offline and the worker is not able to complete its job. |
We also found an easier way to achieve the described end result. In Chrome:
DataStore will send an event indicating that there is nothing to sync, but the un-synced model record will remain stored in the local IndexedDB table with undefined _version etc. attributes. |
The PR with the fix for this issue has been reviewed and approved, soon to be released in the coming weeks. I would keep an eye on that PR, we will later comment here when the fix is officially available. |
@chrisbonifacio Sorry if this was miscommunicated, but that PR doesn't doesn't solve this problem. It solves a problem with But, @iartemiev is starting work on the fix today, which we may initially release under a tag for testing. |
Are there downsides or considerations to take when using optimistic concurrency until this is fixed? I found documentation here about writing a conflict resolution function https://docs.amplify.aws/lib/datastore/conflict/q/platform/js/#custom-configuration Is there any recommended logic to use there until we can go back to using auto-merge? |
@svidgen Just to confirm, will @iartemiev work address the issue with the DataStore missing propagation of local updates to AppSync on spotty Internet connection? Have you been able to reproduce it? The fact that switching conflict resolution to optimistic concurrency helped some users but not others makes me wonder if there are two different issues. |
@duckbytes Not sure if this helps, but switching to optimistic concurrency broke our auto-increment logic where we issue an empty GraphQL update to atomically increment the Counter model _version attribute. To make it work with optimistic concurrency, we would have to query the record first for the latest _version, then make an update with the latest _version value known to client. This could fail if there are many concurrent updates on the same record as it is prone to race conditions. Optimistic concurrency didn't resolve the DataStore issue for us, so we simply switched back to auto-merge. |
Has there been any progress or new information on this issue? Or potential workarounds? I'm having some difficulty with optimistic concurrency and trying to resolve conflicts manually, as well as coming up against bugs (#11708). I'd like to try auto-merge again if possible. |
@duckbytes - We have a plan to improve the experience with Auto Merge but it is still work in progress due to the underlying complexity of the fix. Our recommendation in general, is to use Optimistic Concurrency. I understand that you're facing bug #11708 and we'll investigate that in parallel to unblock you from continuing to use optimistic concurrency. |
Hello @duckbytes, We really appreciate the time and detail you've put into this issue. A fix was merged this week and is now available on This fix will be published to Thanks! |
Thanks very much @stocaaro for your work on this! My web version is still on Amplify V4 because of the breaking changes in DataStore, so I won't be able to switch over to automerge and test until I can do the work to migrate everything over. The smartphone app is on Amplify V5 though, so I will get that up to date soon. |
Sounds good @duckbytes, From the testing I’ve done, I believe this problem is solved in the current V6 release. I’m going to close this issue. If you get your applications updated and are seeing issues that look related in any way, please comment here (or on a new issue linked back to this one) with details. Thanks, |
Before opening, please confirm:
JavaScript Framework
React
Amplify APIs
GraphQL API, DataStore
Amplify Categories
api
Environment information
Describe the bug
When making changes too fast to a record, mutations to the API are missed or send incorrect data.
An observer will also react and return the UI into an old state that reflects the out of date mutation.
The problem is more obvious when on a slow network connection, and in real world testing has caused data to be saved incorrectly with normal usage. When offline, data is synced correctly once a connection is re-established.
I also find the problem occurs when testing with Cypress at full network speed.
I use an observer to make sure that the record has one field set before enabling a button to set the next field. However I have observed this issue when changing a single field repeatedly.
One workaround is to check that _version has incremented before allowing further mutations. However this prevents it from working offline as that value is never incremented until data is sent to the API.
In my example I'm setting two fields: status and a time stamp. But it does seem to happen when setting one field too.
Expected behavior
DataStore should always sync changes to the record and not miss changes. The observer should not update with incorrect data.
Reproduction steps
await DataStore.save(new models.Task({}))
Code Snippet
src/App.js
src/App.css
Log output
aws-exports.js
Manual configuration
No response
Additional configuration
No response
Mobile Device
No response
Mobile Operating System
No response
Mobile Browser
No response
Mobile Browser Version
No response
Additional information and screenshots
A video demonstration (sorry for quality, the webm looked much better but github won't attach it):
output.mp4
An example of where the status field mismatches what was sent to the API (
timeRiderHome
,timeDroppedOff
andtimePickedUp
all being set means the status should beCOMPLETED
). This doesn't always happen, but does often.The entry in dynamoDB where the status doesn't match:
The text was updated successfully, but these errors were encountered: