Handle moveDistinctIds race conditions #524
Conversation
|
Changing the approach from the "inline alias" required adding Also had moved towards |
|
@macobo this should be good for a second look |
src/utils/db/db.ts
Outdated
| @@ -602,6 +603,12 @@ export class DB { | |||
| 'updateDistinctIdPerson' | |||
| ) | |||
|
|
|||
| // this is caused by a race condition and will trigger a retry with | |||
There was a problem hiding this comment.
Suggestion: If the above query returns an error due to primary key violation (person was deleted) then also raise a RaceConditionError.
Then we can only handle RaceConditionErrors in the try-catch rather than retrying everything.
| // re-run alias to ensure the updated persons are fetched and | ||
| // merged safely | ||
| return await this.alias(otherPersonDistinctId, mergeIntoDistinctId, teamId, false, failedAttempts) | ||
| throw error |
There was a problem hiding this comment.
Q: Is the other try-catch still valid? if not, it makes sense to perhaps remove the loop alltogether.
There was a problem hiding this comment.
Should still be valid, as a distinct ID can be added in between moveDistinctIds and deletePerson, in which case we need to move the IDs again so we can delete without an error.
|
I really should put some effort into deflaking these tests 😠 |
…n-server#524) * handle race conditions with moveDistinctIds * update comment * update error message * update test * update approach * updates * add RaceConditionError * address review comments * updates * update tests
Changes
Me and @macobo will probably discuss this a bit further tomorrow.
Handles 2 types of race conditions that may arise when running
mergeDistinctIds:mergeIntoperson no longer exists: this triggers an error and leaves the persons unmergedotherPersonno longer exists: this currently does not trigger an error but leaves persons unmergedThese happen because in between the time the persons are fetched and
moveDistinctIdsis called, another thread may have deleted either of the persons. Happy to explain further.Checklist