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 ongoingDeliveryId bug (duplicates with Verify API) in attestation service #8958

Merged
merged 6 commits into from
Nov 17, 2021

Conversation

eelanagaraj
Copy link
Contributor

@eelanagaraj eelanagaraj commented Nov 11, 2021

Description

Closes #8808

The fix/code changes here is fairly simple, but the decision behind it is a bit more involved, so I've included details on the options considered below:

Options considered (implemented (3))

  1. when sending SMS via verify, check if there is an existing verification for that phone number and if so cancel it
    (--) slow (adds (from rough benchmarking) 0.7 ms before attempting to send the SMS, really not ideal since we are trying as hard as possible to cut time pre-send)
    (-?) possible that this (and (2) below) would mess with Twilio Verify's smart routing and make performance worse
  2. cancel the verify flow immediately after sending it via Verify.
    (+) adds no time to receiving SMS, verify flow from perspective of client;
    (--?) Seems like it should only create the Verification Resource once the message has been sent, but I can't find confirmation that canceling immediately wouldn't in some cases cause the message to not send at all, and there's not great visibility into delivery status here, so bugs here would likely be really insidious/hard to detect going forwards
  3. make the DB index on ongoingDeliveryId non-unique. Only keep these records for DB_RECORD_EXPIRY_MINS anyways and old records are purged.
    (+) just updating the index to be non-null (but keeping the index) should maintain DB performance
    (+) minimal change that should not affect any other areas of the code; highly unlikely to degrade the performance of Verify/other providers sends
    () would need to add a time-ordering constraint to the one area that accesses status by delivery ID (delivery report stuff); though even this is a formality given that the Verify delivery IDs are not used and all the others are already unique
    (-) if more than 5 codes are sent to the same number in 10 min with the Verify API, the sendSMS would fail for Verify API anyways (as it maxes out at 5) -- however, this situation should (if at all) only happen exceedingly rarely (for real use cases; more often on alfajores due to there being only one shared AS under the hood) due to attempt limits, and this should at least result in retrying the send. A possible situation could look like some combination of: in a country where only twilioverify is configured; a bunch of issuers use the same attestation service + twilio config and at least 2 of the selected issuers use the same AS; both messages fail + are resent 3 times (should max out attempts and switch issuer at this point); or perhaps something similar to before + user tries to connect PN to another address within 10 min and is randomly assigned to the same issuers as before.
    (-) if we re-add the uniqueness constraint in the future, the DB migration as is would cause a one-time dip as the delivery report retry logic to not work for stored attestation sends (this should not impact too many attestations; only those sent recently enough to receive a delivery report, and even so would be only for the built-in resend logic)

Drive by changes

  • Improved test mocks (to include update functionality) + restructured unit tests just slightly (when experimenting with + testing out solutions 1/2)

Testing

  • tested database migration (up/down) locally
  • deployed and tested on alfajores; will continue to leave it up for continued testing + examination of logs

@eelanagaraj eelanagaraj force-pushed the eelanagaraj/fix-verify-uniqueness branch 3 times, most recently from 6c37759 to 450b85b Compare November 12, 2021 12:15
@eelanagaraj eelanagaraj force-pushed the eelanagaraj/fix-verify-uniqueness branch from 620fb28 to 05796b9 Compare November 12, 2021 16:46
@eelanagaraj eelanagaraj marked this pull request as ready for review November 12, 2021 17:47
@eelanagaraj eelanagaraj requested review from a team, timmoreton, alecps and codyborn and removed request for a team November 12, 2021 17:47
Copy link
Contributor

@codyborn codyborn left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for outlining the detailed thought process

@eelanagaraj eelanagaraj added the automerge Have PR merge automatically when checks pass label Nov 17, 2021
@mergify mergify bot merged commit cf48bd7 into master Nov 17, 2021
@mergify mergify bot deleted the eelanagaraj/fix-verify-uniqueness branch November 17, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate/fix bug re: ongoingDeliveryId uniqueness constraint with Verify API
2 participants