Stamp webhook rate limiter only on successful delivery - #88
Merged
Conversation
rateLimited previously recorded lastSent[key] at check time, before deliver() ran. If that delivery then failed all retries, the next firing of the same metric within notify_min_interval_seconds was still suppressed as a "repeat" even though nothing was ever delivered. Split the stamp out of the check: rateLimited() now only reads lastSent, and dispatch() calls the new recordSent() after deliver() reports success. deliver() now returns whether it ultimately succeeded so dispatch() can make that call. Closes #62
|
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
2 participants
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.
📖 Description
rateLimited(internal/alert/notify.go) stampedlastSent[key] = ev.Atatcheck time, before
deliverran. If that delivery then failed all retries,the next firing of the same metric within
notify_min_interval_secondswasstill suppressed as a "repeat" — even though nothing was ever delivered.
The fix splits the check from the stamp:
rateLimitednow only readslastSent, and a newrecordSentis called fromdispatchonly afterdeliverreports the delivery ultimately succeeded.delivernow returns aboolfor that purpose instead ofvoid.This is a fix, not a breaking change; no config or API shape changes.
🎫 Issues
Closes #62
👩💻 Reviewer Notes
The core change is small:
delivergained aboolreturn value, and therate-limit stamping moved out of
rateLimitedinto a newrecordSenthelper called from
dispatchonly on success.lastSentis still onlytouched by the single worker goroutine, so no locking changes were needed.
📑 Test Plan
Added
TestNotifier_RateLimitDoesNotCountFailedDeliveryininternal/alert/notify_test.go: an event delivery that fails (500 response,0 retries) must not stamp the rate limiter, so a second firing of the same
metric 1s later — deep inside a 60s rate-limit window — is still delivered
once the endpoint recovers. Verified this test fails on the pre-fix code
(the second event gets dropped as rate-limited) and passes with the fix.
Ran locally:
go build ./...go vet ./...go test ./... -race -cover(all packages pass;internal/alertat 92.8%coverage)
golangci-lint run(0 issues)✅ Checklist
General
go test ./... -race -coverpasses locally).go vet ./...andgolangci-lint runare clean.ARCHITECTURE.mdif this changes a documented design decision. (not applicable — internal robustness fix, no design change)REST API / configuration / packaging
Not applicable — no REST API, configuration, or packaging changes.
⏭ Next Steps
None.