🐛 Fix limiter middleware not counting fiber.NewErrorf responses as failed requests#3623
🐛 Fix limiter middleware not counting fiber.NewErrorf responses as failed requests#3623ReneWerner87 merged 5 commits intomainfrom
Conversation
…nted by limiter Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
…s failed requests Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3623 +/- ##
==========================================
+ Coverage 91.13% 91.14% +0.01%
==========================================
Files 112 112
Lines 11296 11310 +14
==========================================
+ Hits 10295 10309 +14
Misses 744 744
Partials 257 257
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot please fix the linting |
Co-authored-by: ReneWerner87 <7063188+ReneWerner87@users.noreply.github.com>
Fixed the linting issues in commit 2ae3c5b. Addressed formatting problems and renamed unused parameters in test functions from |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the limiter middleware where fiber.NewErrorf responses were not being properly counted as failed requests when SkipSuccessfulRequests: true and SkipFailedRequests: false. The issue occurred because the middleware was checking the response status code before Fiber's error handler processed the error, causing failed requests to be incorrectly skipped.
- Added a
getEffectiveStatusCode()helper function that checks for*fiber.Errortypes and uses their code - Updated both sliding window and fixed window limiters to use the new status code detection logic
- Added comprehensive tests to verify the fix works for both limiter types
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| middleware/limiter/limiter.go | Adds the getEffectiveStatusCode() helper function to properly detect status codes from fiber errors |
| middleware/limiter/limiter_sliding.go | Updates sliding window limiter to use the new status code detection for skip logic |
| middleware/limiter/limiter_fixed.go | Updates fixed window limiter to use the new status code detection for skip logic |
| middleware/limiter/limiter_test.go | Adds test cases to verify the bug fix for both sliding and fixed window limiters |
| package limiter | ||
|
|
||
| import ( | ||
| "errors" |
There was a problem hiding this comment.
[nitpick] The import should be grouped with standard library imports. Add a blank line after this import to separate it from the third-party imports below.
The limiter middleware was not properly counting requests that return
fiber.NewErrorf(...)as failed requests whenSkipSuccessfulRequests: trueandSkipFailedRequests: false.Problem
When a handler returns
fiber.NewErrorf(fiber.StatusInternalServerError, "Error"), the limiter middleware would checkc.Response().StatusCode()to determine if the request should be skipped. However, at this point the error hasn't been processed by Fiber's error handler yet, soc.Response().StatusCode()returns 200 instead of the expected 500 status code from the error.This caused failed requests to be incorrectly skipped when
SkipSuccessfulRequests: true, breaking rate limiting for error responses.Example
Before: Repeated requests would all return 500 (not rate limited)
After: First request returns 500, subsequent requests return 429 (properly rate limited)
Solution
Added a
getEffectiveStatusCode()helper function that:*fiber.Errorusingerrors.As()Codefield as the status codec.Response().StatusCode()Both sliding window and fixed window limiters now use this helper to properly determine the effective status code for skip logic.
Testing
Fixes #3622.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
exampleretry.com/tmp/go-build1721103930/b273/client.test -test.paniconexit0 -test.v=test2json -test.timeout=10m0s -test.count=1 -test.shuffle=on(dns block)/tmp/go-build65591961/b271/client.test -test.testlogfile=/tmp/go-build65591961/b271/testlog.txt -test.paniconexit0 -test.timeout=1m0s -test.failfast=true(dns block)/tmp/go-build2682536009/b273/client.test -test.paniconexit0 -test.v=test2json -test.timeout=10m0s -test.count=1 -test.shuffle=on(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.