🐛 bug: Fixes and improvements for limiter middleware#3899
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughNormalize expiration checks to use duration directly and ceil-rounded seconds (minimum 1s); refactor fixed and sliding limiter logic to use timestamp-based windows, recompute expirations/TTLs and hit migrations, add timestamp updater, and add extensive concurrency- and TTL-aware tests. Minor formatting tweaks in idempotency files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Limiter
participant Timer
participant Storage
Client->>Limiter: Incoming request
Limiter->>Timer: Read shared timestamp
Limiter->>Storage: Get entry
alt Missing entry or window expired
Limiter->>Limiter: Initialize window (expiration = ceil(sec) ≥ 1s)
Limiter->>Limiter: Reset hit counters, compute resetInSec, remaining
else Entry exists within window
Limiter->>Limiter: Compute elapsed, migrate/reset/move curr→prev as needed
Limiter->>Limiter: Recalculate weight, rate, resetInSec, remaining
end
Limiter->>Storage: Persist entry with TTL = resetInSec + expiration
alt Limit exceeded
Limiter->>Client: Respond 429
else Within limit
Limiter->>Client: c.Next (allow)
Client->>Limiter: Response outcome (success/fail)
Limiter->>Limiter: Apply skip-success/skip-failure logic (same timestamp-based transitions)
Limiter->>Storage: Persist updated entry with adjusted TTL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–90 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2024-06-30T00:38:06.580ZApplied to files:
📚 Learning: 2024-10-12T10:01:44.206ZApplied to files:
📚 Learning: 2024-10-08T19:06:06.583ZApplied to files:
📚 Learning: 2024-10-02T23:03:31.727ZApplied to files:
📚 Learning: 2025-10-16T07:19:52.418ZApplied to files:
📚 Learning: 2024-12-13T08:14:22.851ZApplied to files:
📚 Learning: 2024-11-29T12:37:27.581ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (4)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness of the rate limiting middleware by addressing edge cases related to skipped requests and limiter expiration. The changes ensure that rate limiters maintain accurate state, prevent erroneous negative counts, and correctly reset their windows, particularly when requests are processed over extended periods or when using sub-second expiration configurations. The added tests provide strong validation for these critical behavioral corrections. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical bug where rate limiter counts could become incorrect for skipped requests, especially when those requests are long-running and outlive the limiter's expiration window. The changes in both the fixed and sliding window implementations are robust, and the addition of regression tests is excellent. I've included a few suggestions to enhance maintainability by reducing some code duplication and pointed out a potential issue in one of the new tests.
| if secondTTL < firstTTL { | ||
| t.Fatalf("expected skip branch to preserve TTL, got first=%s second=%s", firstTTL, secondTTL) | ||
| } |
There was a problem hiding this comment.
This test assertion appears to be incorrect. For the sliding window limiter, the TTL passed to the storage is calculated as time.Duration(resetInSec+expiration)*time.Second, where resetInSec = e.exp - ts.
When a request is processed, there are two calls to storage.Set: one before c.Next() (for incrementing) and one after (for decrementing, in the case of skipped requests). The timestamp ts will be greater for the second call. This means resetInSec will be smaller, and therefore secondTTL will naturally be smaller than firstTTL.
The current assertion if secondTTL < firstTTL will cause the test to fail, but the failure message "expected skip branch to preserve TTL" suggests you expect them to be equal or for secondTTL to be greater.
The implementation's TTL calculation seems correct for ensuring the storage entry expires at a fixed absolute time, so the decreasing TTL duration is expected. This test might need to be adjusted to reflect this expected behavior, for instance, by asserting that secondTTL is less than firstTTL but within an expected range.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
middleware/limiter/limiter_test.go (3)
69-71: Rename unused receiver to_.Static analysis flagged the unused receiver. Since
Closedoesn't need to access the storage state, rename the receiver.-func (s *recordingStorage) Close() error { +func (*recordingStorage) Close() error { return nil }
935-939: Fix variable naming to follow Go conventions.Static analysis flagged
baseTsshould bebaseTSper Go's style guide for initialisms.- baseTs := uint32(time.Now().Unix()) + baseTS := uint32(time.Now().Unix()) refreshTimestamp := func(ts uint32) { atomic.StoreUint32(&utils.Timestamp, ts) } - refreshTimestamp(baseTs) + refreshTimestamp(baseTS)Apply the same change at lines 951 and 958 where
baseTsis referenced.
1033-1037: Same naming fix needed here.Apply the same
baseTs→baseTSrename as noted for the fixed window test.- baseTs := uint32(time.Now().Unix()) + baseTS := uint32(time.Now().Unix()) refreshTimestamp := func(ts uint32) { atomic.StoreUint32(&utils.Timestamp, ts) } - refreshTimestamp(baseTs) + refreshTimestamp(baseTS)Also update references at lines 1049 and 1056.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
middleware/limiter/config.go(1 hunks)middleware/limiter/limiter_fixed.go(4 hunks)middleware/limiter/limiter_sliding.go(4 hunks)middleware/limiter/limiter_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
middleware/limiter/limiter_test.go
🧬 Code graph analysis (3)
middleware/limiter/limiter_test.go (1)
utils/time.go (1)
Timestamp(13-13)
middleware/limiter/limiter_fixed.go (1)
utils/time.go (1)
Timestamp(13-13)
middleware/limiter/limiter_sliding.go (1)
utils/time.go (1)
Timestamp(13-13)
🪛 GitHub Check: lint
middleware/limiter/limiter_test.go
[warning] 69-69:
unused-receiver: method receiver 's' is not referenced in method's body, consider removing or renaming it as _ (revive)
[failure] 1033-1033:
ST1003: var baseTs should be baseTS (stylecheck)
[failure] 935-935:
ST1003: var baseTs should be baseTS (stylecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build (1.23.x, macos-latest)
- GitHub Check: Build (1.23.x, windows-latest)
- GitHub Check: Build (1.22.x, macos-latest)
- GitHub Check: Build (1.19.x, windows-latest)
- GitHub Check: Build (1.20.x, windows-latest)
- GitHub Check: Build (1.21.x, windows-latest)
- GitHub Check: Build (1.20.x, macos-latest)
- GitHub Check: Build (1.18.x, macos-latest)
- GitHub Check: Build (1.17.x, macos-latest)
- GitHub Check: Compare
🔇 Additional comments (14)
middleware/limiter/config.go (1)
115-117: LGTM!The simplified check
cfg.Expiration <= 0correctly validates non-positive durations. This aligns well with the new ceiling-based expiration model in the limiter implementations, which will round sub-second expirations up to at least 1 second.middleware/limiter/limiter_fixed.go (3)
22-27: LGTM!The ceiling-based expiration calculation with minimum 1-second enforcement is a sound approach. This ensures sub-second configurations don't result in immediate expiration and provides a defensive safeguard.
73-73: LGTM!Using the computed
expirationvalue for storage TTL ensures consistency with the actual window duration.
98-115: Core fix correctly implemented.The skip branch now properly refreshes the timestamp and recomputes window state before decrementing. The guard
if e.currHits > 0prevents negative hit counts after window expiration during long-running requests. This addresses the PR objective.middleware/limiter/limiter_sliding.go (3)
22-27: LGTM!Consistent ceiling-based expiration calculation with the fixed window implementation.
57-72: LGTM!The window transition logic correctly handles both full expiration (resetting both counters) and partial expiration (migrating current to previous). This ensures the sliding window algorithm maintains accurate state.
127-158: Core sliding window fix correctly implemented.The
movedToPrevflag correctly tracks whether the window rolled during request processing, ensuring the decrement targets the appropriate counter (prevHitsvscurrHits). The guards prevent negative counters in all edge cases.middleware/limiter/limiter_test.go (7)
547-581: Good regression test for the core fix.This test correctly validates that the fixed window limiter handles long-running requests that span expiration boundaries. The sequence (successful skipped → failed counted → rate limited) confirms the expiration reset behavior works as expected.
664-698: Good regression test for sliding window.Mirrors the fixed window test for the sliding window implementation, ensuring consistent behavior across both limiter types.
741-783: TTL preservation test is valuable.This test verifies that the skip branch doesn't inadvertently shorten the storage TTL, which would cause premature garbage collection of rate limit state.
324-351: LGTM!Clean test for basic sliding window reset behavior after expiration.
965-994: LGTM!Good test verifying that both
Setcalls (initial and skip branch) use consistent TTL values.
996-1026: LGTM!Validates that the ceiling-rounded 1500ms expiration (→ 2 seconds) persists correctly across window boundaries.
18-48: LGTM!The
recordingStorageimplementation is well-structured for test purposes with proper mutex protection and defensive copying. Based on learnings, the project uses testify for assertions, but the custom storage helper is appropriate here for capturing TTL values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
middleware/limiter/limiter_test.go (1)
741-783: TTL expectation in Sliding_Window_Skip_TTL_Preservation is likely invertedThe logic in
Test_Limiter_Sliding_Window_Skip_TTL_Preservationcurrently fails the test whensecondTTL < firstTTLand the failure message claims “expected skip branch to preserve TTL”. For the current SlidingWindow implementation, the TTL passed to storage is derived from the remaining window (resetInSec) plus the expiration, so on the secondSetcall (afterc.Next()), the effective timestamp is later and the TTL should naturally be smaller, not larger.Unless you’ve intentionally changed the TTL semantics to keep it constant across these two
Setcalls, this assertion is backwards and will fail under correct behavior.Consider inverting the condition to assert that the TTL decreases:
- if secondTTL < firstTTL { - t.Fatalf("expected skip branch to preserve TTL, got first=%s second=%s", firstTTL, secondTTL) - } + if secondTTL >= firstTTL { + t.Fatalf("expected subsequent TTL to shrink as time advances, got first=%s second=%s", firstTTL, secondTTL) + }This keeps the useful check that the initial TTL exceeds the configured expiration while aligning the comparison with the time-based TTL calculation.
🧹 Nitpick comments (1)
middleware/limiter/limiter_test.go (1)
18-71: recordingStorage helper looks good; adjust Close receiver to satisfy linterThe in-test
recordingStorageimplementation is concurrency-safe and fits the storage interface well (byte copies, mutex protection, TTL recording). The only issue is the linter warning about the unused receiver inClose.You can silence this without changing behavior by dropping the receiver name:
-func (s *recordingStorage) Close() error { +func (*recordingStorage) Close() error { return nil }This keeps the method on
*recordingStoragefor interface compatibility while resolving theunused-receiverwarning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/limiter/limiter_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
middleware/limiter/limiter_test.go
🧬 Code graph analysis (1)
middleware/limiter/limiter_test.go (1)
utils/time.go (1)
Timestamp(13-13)
🪛 GitHub Check: lint
middleware/limiter/limiter_test.go
[warning] 69-69:
unused-receiver: method receiver 's' is not referenced in method's body, consider removing or renaming it as _ (revive)
[failure] 931-931:
File is not gofmt-ed with -s (gofmt)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Build (1.22.x, macos-latest)
- GitHub Check: Build (1.23.x, windows-latest)
- GitHub Check: Build (1.21.x, macos-latest)
- GitHub Check: Build (1.23.x, ubuntu-latest)
- GitHub Check: Build (1.22.x, windows-latest)
- GitHub Check: Build (1.21.x, ubuntu-latest)
- GitHub Check: Build (1.20.x, macos-latest)
- GitHub Check: Build (1.20.x, windows-latest)
- GitHub Check: Build (1.19.x, macos-latest)
- GitHub Check: Build (1.18.x, ubuntu-latest)
- GitHub Check: Build (1.17.x, ubuntu-latest)
- GitHub Check: Build (1.17.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (2)
middleware/limiter/limiter_test.go (2)
324-351: Sliding-window reset-after-expiration test matches the reported bug scenario
Test_Limiter_Sliding_Window_Reset_After_Expiration(Max=1, 1s expiration, then a ~3s sleep) correctly exercises the bug from #3892: limiter hits 429 on the second immediate request, then recovers to 200 after the window has clearly expired. The flow and expectations look correct and give good regression coverage.
547-581: Long-running request + SkipSuccessfulRequests tests exercise critical edge case wellBoth
Test_Limiter_Fixed_Window_Skip_Long_Request_ExpirationandTest_Limiter_Sliding_Window_Skip_Long_Request_Expirationmodel:
- A first request that succeeds but runs longer than the expiration window.
- Subsequent failing requests that should be counted and eventually limited.
- Use of
app.Test(..., -1)plustime.Sleepto allow the long request to complete.The expectations (200 → 500 → 429) align with the intended semantics of
SkipSuccessfulRequestsand ensure long-running successes don’t leave the limiter in a permanently exceeded state. This is a solid regression harness for the described bug.Also applies to: 664-698
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/limiter/limiter_test.go (1)
741-783: TTL recording test is precise; be aware it tightly couples to internal TTL semanticsUsing
recordingStorageand capturingexpirationsunder the mutex is a solid way to assert on sliding-window TTL behavior when skipping successful requests. The checks:
- At least two
Setcalls recorded.- Initial TTL strictly greater than
Expiration.- Second TTL not less than the first.
encode fairly specific expectations about how the skip branch updates TTLs. That’s fine as a regression test, but keep in mind it will need adjustment if future internal TTL calculations change (even if external behavior stays equivalent).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
middleware/idempotency/response_msgp.go(2 hunks)middleware/idempotency/response_msgp_test.go(1 hunks)middleware/limiter/limiter_test.go(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- middleware/idempotency/response_msgp.go
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/limiter/limiter_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
middleware/limiter/limiter_test.go
🪛 GitHub Check: lint
middleware/limiter/limiter_test.go
[warning] 69-69:
unused-receiver: method receiver 's' is not referenced in method's body, consider removing or renaming it as _ (revive)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build (1.23.x, macos-latest)
- GitHub Check: Build (1.23.x, windows-latest)
- GitHub Check: Build (1.19.x, macos-latest)
- GitHub Check: Build (1.21.x, windows-latest)
- GitHub Check: Build (1.20.x, windows-latest)
- GitHub Check: Build (1.21.x, ubuntu-latest)
- GitHub Check: Build (1.17.x, windows-latest)
- GitHub Check: Build (1.17.x, macos-latest)
- GitHub Check: Compare
🔇 Additional comments (9)
middleware/idempotency/response_msgp_test.go (1)
3-4: Generated test formatting change is fineThe added blank line after the package declaration keeps this generated test file gofmt-compliant and doesn’t affect behavior. No further changes needed.
middleware/limiter/limiter_test.go (8)
324-351: Good regression test for sliding window reset after expirationThis test clearly reproduces the “stuck 429” bug for
SlidingWindowand verifies that afterExpirationelapses (with a generous 3s sleep for a 1s window), the limiter allows requests again. The configuration and expectations match the linked issue’s goal.
547-581: Fixed window long-request + SkipSuccessfulRequests scenario is well coveredThis test nicely captures the case of a long-running successful request in a 1s fixed window with
SkipSuccessfulRequests=true, then a failing request within the same window, and finally a 429 once the failing request has “consumed” the slot. Usingapp.Test(..., -1)avoids client timeouts while the handler sleeps.
664-698: Sliding window long-request + SkipSuccessfulRequests parity with fixed windowThis is a good sliding-window counterpart to the fixed-window long-request test: same configuration and expectations (200, then 500, then 429), ensuring both implementations agree on how long-running successful requests and subsequent failures interact with the window.
930-963: Fixed window sub-second expiration test: good use of Timestamp without t.ParallelThis test cleanly simulates a 500ms window by pinning
utils.Timestampviaatomic.StoreUint32, restoring it withdefer, and not callingt.Parallel(), which helps avoid surprises with the global timestamp. It verifies both a successful first request and the presence ofX-RateLimit-Reset, then a 429 on the second request at the same logical timestamp, which is exactly what you want for the sub-second normalization behavior.
965-995: Fractional fixed-window TTL sync test is clear and robustThis test uses
recordingStorageto assert that for a 500ms fixed window withSkipSuccessfulRequests=true, bothSetcalls record a TTL of exactlytime.Second, confirming that fractional expirations are normalized consistently. The use oft.Parallel()is fine here since no global state is mutated and storage is test-local.
996-1032: Fixed window fractional expiration window-persistence behavior is well specifiedThe
1500 * time.Millisecondcase with manualutils.Timestampcontrol accurately describes desired behavior:
- First request at
baseTSsucceeds.- Second request at
baseTS+1is still rate-limited (429), confirming the window persists across the fractional boundary.- Third request at
baseTS+3succeeds, confirming reset after the window plus normalization.Not running this test with
t.Parallel()is appropriate given the global timestamp mutation.
1034-1067: Sliding window sub-second expiration mirror test looks correctThis is the sliding-window mirror of the fixed-window sub-second test, with the same
utils.Timestamppinning pattern and 500ms expiration. It validates that:
- The first request succeeds and sets a non-empty
X-RateLimit-Reset.- A second request at the same logical timestamp gets 429.
Again, avoiding
t.Parallel()here is the right call given the global timestamp manipulation.
18-72: recordingStorage implementation is sound; apply suggested lint fix to Close receiverThe in-memory
recordingStoragewith mutex protection, defensive copies inGet/Set, and expiration tracking is appropriate for testing TTL behavior under concurrency.The unused receiver in
Close()can be fixed by omitting the receiver name:-func (s *recordingStorage) Close() error { +func (*recordingStorage) Close() error { return nil }This satisfies linting rules and clearly signals the receiver isn't used while maintaining interface compliance.
Summary
Fixes #3892