Key bad-credentials notice cooldowns by error code#33
Merged
Conversation
Follow-up to the resend-cooldown fix (which addressed a different bug: sentBadCredentialsNotice never resetting since handlePossible40002 doesn't log the user out). That fix's cooldown didn't distinguish which kind of bad-credentials event was last notified: if a 40002 notice sent successfully, a real 4004 full logout arriving within the following hour would be suppressed by the 40002's resend cooldown -- leaving the user with stale "you do not need to log in" advice even after their token was actually cleared and login is now required. badCredentialsNoticeAttemptDecision now takes both the incoming and last-notified error codes and only applies either cooldown when they match; a different code always gets through immediately, since it may carry materially different (even opposite) recovery instructions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refines the bad-credentials notice cooldown logic so the cooldown only suppresses repeat notices for the same underlying Discord error code, preventing stale/incorrect recovery instructions from being shown when a different invalidation scenario occurs shortly after.
Changes:
- Track
lastBadCredentialsErrorCodeonUserand reset it on successfulLogin. - Update
badCredentialsNoticeAttemptDecisionto bypass cooldowns when the incoming error code differs from the last-notified code. - Extend unit tests to cover cooldown bypass when error codes differ.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| user.go | Adds per-last-error-code tracking and updates cooldown gating to only apply when error codes match. |
| user_notice_test.go | Adds test cases validating that differing error codes bypass active cooldowns. |
Comments suppressed due to low confidence (1)
user.go:2378
time.Now()is called twice while holdingbridgeStateLock(once for the cooldown decision and again when recordinglastBadCredentialsNoticeAttempt). Using a single timestamp avoids minor inconsistencies and makes the state update easier to reason about (especially around cooldown boundaries).
attempt, skipReason := badCredentialsNoticeAttemptDecision(user.sentBadCredentialsNotice, errorCode, user.lastBadCredentialsErrorCode, user.lastBadCredentialsNoticeAttempt, time.Now())
if !attempt {
user.bridgeStateLock.Unlock()
log.Debug().Bool("notice_sent", false).Msg("Bad credentials event (" + skipReason + ")")
return
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #32, which merged before this was caught by a delayed Codex review pass on that PR (linking here since #32's own thread is now closed): comment.
That PR's resend-cooldown fix (itself a fix for a different bug:
sentBadCredentialsNoticenever resetting sincehandlePossible40002doesn't log the user out) didn't distinguish which error code was last notified. Concretely: if a 40002 notice sends successfully, a real 4004 full logout arriving within the following hour would get suppressed by the 40002's resend cooldown — leaving the user staring at "you do not need to log in" advice even though their token was actually just cleared and they do need to log in now.badCredentialsNoticeAttemptDecisionnow takes both the incoming and last-notified error codes; the cooldowns only apply when they match. A different code always gets through immediately, since it may carry materially different (even opposite) recovery instructions.Test plan
go build ./.../go vet ./.../gofmt— cleango test ./... -race— clean🤖 Generated with Claude Code