getodk / central-backend

Node.js based backend for ODK Central
https://docs.getodk.org/central-intro/
Apache License 2.0
50 stars 76 forks source link

Prevent concurrent changes to same entity from different submissions #1195

Closed matthew-white closed 1 month ago

matthew-white commented 2 months ago

This PR adds a solution for the race condition described in getodk/central#705.

What has been done to verify that this works as intended?

I'm able to reproduce the race condition in testing. However, I'm still working on writing a test that feels sufficiently reliable and not like its own race condition. After this PR is merged, I'll follow up with another PR that adds a test.

Why is this the best possible solution? Were any other approaches considered?

I wanted to reuse the _lockEntity() function, which I think was introduced in #1033. I had to make one change to the function in this PR: see the code comments in the diff.

There was a little bit of a question of timing: when exactly should the entity be locked? I decided to lock the entity in the main body of Entities._processSubmissionEvent() and not in Entities._createEntity() or Entities._updateEntity(), even though there are cases where the latter two functions return early and never read or write to the entity. I decided to take that approach because it's possible for both Entities._createEntity() and Entities._updateEntity() to be run: if both create="true" and update="true" are specified, then Entities._createEntity() will be attempted if Entities._updateEntity() fails. I wanted to avoid a situation where multiple locks were attempted on the same entity.

Before submitting this PR, please make sure you have:

matthew-white commented 1 month ago

Locking the entity caused a few existing tests to fail, specifically because the UUID validation was moved to earlier in the entity processing. That's because the UUID is needed to lock the entity (see #1200 for details). I temporarily skipped the failing tests before merging the PR, but I unskipped them in #1196.