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: resolve intermittent Tokenserver test failure #1171

Merged
merged 11 commits into from
Nov 22, 2021

Conversation

ethowitz
Copy link
Contributor

Description

Add time.sleeps to fix an intermittent timestamp issue in a unit test

Testing

The failing test should pass on CI

Issue(s)

Closes #1170

@ethowitz ethowitz requested a review from a team November 18, 2021 20:14
@ethowitz ethowitz marked this pull request as draft November 18, 2021 20:37
@ethowitz ethowitz removed the request for review from a team November 18, 2021 20:37
@ethowitz
Copy link
Contributor Author

This is still not working...converting to a draft

@ethowitz
Copy link
Contributor Author

Whew. The problem was that users were sometimes being created with the same timestamp, which caused a problem here: since the two most recently created users share the same timestamp, neither of those users were being replaced, resulting in two "active" user records for a given email address.

@ethowitz ethowitz marked this pull request as ready for review November 19, 2021 20:02
@ethowitz ethowitz requested a review from a team November 19, 2021 20:02
@jrconlin
Copy link
Member

oh, huh.
I hate to say it, but I think I remember a similar issue cropping up with Sync. Basically, things were working far faster in the current environment and what used to take a few nanos to complete was effectively being done in parallel. Guessing you hit that too.

@@ -162,20 +162,32 @@ def test_cleanup_of_old_records(self):
# Do a sleep halfway through so we can test use of grace period.
email1 = '[email protected]'
user1 = self.database.allocate_user(email1)
# We have to sleep between every user create/update operation: if two
Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if this might eventually bite us (e.g. if the client is also in rust and is on a particularly close node, they may respond faster than one ms.) We might want to add an issue to check for active user records before writing a second and returning a 409 CONFLICT if one is detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that the risk of this happening on the new Tokenserver is any greater than the risk of it happening on the old Tokenserver 🤔 Requests on the new Tokenserver will still take well over 1 ms to complete, so I think the only way this could happen would be if a client decided to bombard us with requests, and I think the old Tokenserver is just as vulnerable to that

@ethowitz ethowitz merged commit 0c05e99 into master Nov 22, 2021
@ethowitz ethowitz deleted the fix/1170-fix-tokenserver-test-failure branch November 22, 2021 22:19
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.

Fix Tokenserver test failure
2 participants