Skip to content

Fix database race condition when tracking OTP requests#5529

Merged
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/fix-otp-race-condition
Oct 21, 2021
Merged

Fix database race condition when tracking OTP requests#5529
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/fix-otp-race-condition

Conversation

@mitchellhenke
Copy link
Contributor

OtpRequests has a unique index on [phone_fingerprint, phone_confirmed], so we get exceptions (NewRelic link) due to the race condition when two requests try to create the row in a short time period.

This PR makes the change use create_or_find_by which attempts to create the row and rescues if the unique index if the unique constraint is violated and then issues a select to get that row. This is similar to the work done in #5341

There is a slight behavior change in this in that the row will have nil for otp_last_sent_at when it is initially created, but this seems to be safe at first glance since we call increment when an OTP is sent.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/fix-otp-race-condition branch from ac52e99 to 012bb4f Compare October 20, 2021 20:31
@mitchellhenke mitchellhenke marked this pull request as ready for review October 21, 2021 14:11
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this looks like it reduces test coverage but we're relying on this new create_or_find to itself rely on DB indexes and be smarter, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can just inline this now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

@mitchellhenke
Copy link
Contributor Author

LGTM, this looks like it reduces test coverage but we're relying on this new create_or_find to itself rely on DB indexes and be smarter, right?

create_or_find_by is new in Rails 6, and has similar behavior with hopefully less lines. It does rely on the unique database index to rescue the create if it fails on the unique constraint so it can fall back to the find_by.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/fix-otp-race-condition branch from 012bb4f to 356ce57 Compare October 21, 2021 16:10
@mitchellhenke mitchellhenke merged commit ba4940d into main Oct 21, 2021
@mitchellhenke mitchellhenke deleted the mitchellhenke/fix-otp-race-condition branch October 21, 2021 16:22
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.

2 participants