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

Preventing the ot-names cycling bug #352

Merged
merged 4 commits into from Apr 15, 2019

Conversation

Projects
None yet
4 participants
@SebastiaanZ
Copy link
Member

commented Apr 15, 2019

This PR is meant to solve the bug in #351 by sleeping until 1 minute after midnight instead of aiming for midnight itself. I've outlined the rationale for this in the linked issue. I'm not entirely sure if this is actually the problem, but given the evidence/audit log times we have, this may very well be the cause.

Since the effect of this bug is something that Discord will probably call "API abuse" (rapid API calls not triggered by user action), we should probably try to merge this soon.

Edit: I've found the root cause of the issue

The problem is that in this step:

seconds_to_sleep = (next_midnight - datetime.utcnow()).seconds

We truncrate the time delta needed to reach midnight down to the seconds, which means we "wake up" slightly before midnight (< 1 second). This is demonstrated here:

code:

from datetime import datetime, timedelta

mn = datetime.utcnow().replace(microsecond=0, second=0, minute=0, hour=0)
mn2 = mn + timedelta(days=1)
delta_seconds = (mn2 - datetime.utcnow()).seconds
delta_full = mn2 - datetime.utcnow()

print(delta_full)
print(delta_seconds)
print(delta_full - timedelta(seconds=delta_seconds))

print(datetime.utcnow() + timedelta(seconds=delta_seconds))

output:

16:50:59.998919              # Full time needed to reach midnight
60659                        # Truncated down to seconds
0:00:00.998919               # The part we're missing by truncating the delta
2019-04-15 23:59:59.001132   # When we'll wake up

To fix this, I'm now simply adding one second to the computed sleep time so we always go over midnight:

seconds_to_sleep = (next_midnight - datetime.utcnow()).seconds + 1

This now closes #351, as the bug was found and fixed.

SebastiaanZ added some commits Apr 15, 2019

@MarkKoz
Copy link
Contributor

left a comment

Should this PR just be squashed? I think so.

@SebastiaanZ

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Should this PR just be squashed? I think so.

Oh, right. I tried to revert first, but apparently I did not succeed. My git-fu is not that honed. How do we go about this?

@MarkKoz

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

To revert I usually do an interactive rebase since that is what I am familiar with, but git revert is the command. I did initially think that the initial commit could be reverted to and we'd call it a day, but I see that you added a second instead of adding a minute like you were initially. Therefore, even if you reverted, it'd still involve and additional commit to change from minute to second.

That being said I still wouldn't have bothered with cleaning up my suggestion since you were just going to scrap the entire thing anyway (and if you had already committed the clean-up but not pushed, you could have dropped the commit through an interactive rebase or other means).

I do think it should be squashed anyway since we went through three iterations of fixing this bug. I don't think it would be beneficial to anyone viewing the history to include those previous attempts.

@lemonsaurus

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

I personally don't really think micromanaging commit history in this way is something we should be spending any amount of time on. four commits for a bugfix is fine.

@SebastiaanZ SebastiaanZ changed the title Preventing the ot-names cycling bug by sleeping until one minute past midnight Preventing the ot-names cycling bug Apr 15, 2019

@sco1

sco1 approved these changes Apr 15, 2019

@SebastiaanZ SebastiaanZ merged commit 0329bbc into master Apr 15, 2019

@SebastiaanZ SebastiaanZ deleted the fix-otnames-cycling-bug branch Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.