🐛 fix(limiter): correct fixed-window hit credit on skipped requests

[gaby]

When SkipSuccessfulRequests/SkipFailedRequests credits a hit back after
the handler runs, the fixed-window strategy decremented currHits
unconditionally and bumped a stale local counter for the
X-RateLimit-Remaining header. If the window rolled over (or another
request reset the entry) between the increment and the credit, currHits
could underflow below zero, inflating the next window's allowance, and
the header reported a value disconnected from stored state.

Mirror the sliding-window semantics: only credit the hit when the entry
still belongs to the same window and currHits is positive, and recompute
remaining from the fresh entry instead of incrementing the stale local.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This PR fixes resource lifecycle and correctness across middlewares: CSRF and session middleware properly release pooled resources, ETag matching implements RFC 9110 weak-tag comparison with new parsing helpers and tests, and rate limiters prevent negative remaining values across window boundaries and skip-success credit-back scenarios.

Changes

Middleware Resource Lifecycle & RFC Compliance Fixes

Layer / File(s)	Summary
Session resource lifecycle release middleware/csrf/session_manager.go, middleware/session/middleware.go	CSRF session manager defers Release() on sessions in getRaw, setRaw, and delRaw. Session middleware skips saveSession() for destroyed sessions and releases them back to the pool instead.
ETag RFC 9110 weak-tag matching middleware/etag/etag.go, middleware/etag/etag_test.go	Response ETag is set before evaluating If-None-Match. New isNoneMatch helper parses/trims entries with wildcard and list support; etagWeakMatch implements RFC 9110 weak comparison with W/ prefix stripping and quote validation. Test_ETag_WeakComparison covers weak/strong combinations and unquoted scenarios.
Rate limiter remaining value clamping middleware/limiter/limiter_fixed.go, middleware/limiter/limiter_sliding.go, middleware/limiter/limiter_test.go	Fixed-window limiter captures window expiration and only credits back if entry belongs to same window; recalculates remaining as maxRequests - currHits and clamps to 0. Sliding-window limiter clamps remaining to 0 after rate computation. Test verifies credit-back does not underflow after window rollover.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• gofiber/fiber#3899: Both PRs modify limiter fixed and sliding window implementations to adjust remaining/hit-counter recalculation across skip and rollover scenarios.
• gofiber/fiber#3893: Both PRs adjust remaining clamping in middleware/limiter/limiter_sliding.go to prevent negative rate-limit header values.
• gofiber/fiber#3016: Both PRs modify session middleware's lifecycle handling and resource cleanup for destroyed sessions.

Suggested labels

codex

Suggested reviewers

• sixcolors
• ReneWerner87
• efectn

Poem

🐰 Sessions freed with careful defer,
ETags matched just as they occur,
Windows roll and limits stay,
No negative values lead astray,
Fiber's middleware hops with cheer!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The PR description clearly explains the problem, root cause, and solution, but does not follow the provided template structure with required sections like 'Changes introduced' and 'Checklist'.	Restructure the description to follow the repository's template, including 'Changes introduced', 'Type of change' checkboxes, and 'Checklist' items to ensure consistency with repository standards.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the specific bug being fixed in the fixed-window rate limiter's hit credit logic when requests are skipped.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/bug-fixes-gg4i5j

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.52%. Comparing base (e187cad) to head (35d6a76).

Files with missing lines	Patch %	Lines
middleware/limiter/limiter_fixed.go	66.66%	1 Missing and 1 partial ⚠️
middleware/limiter/limiter_sliding.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4422      +/-   ##
==========================================
+ Coverage   91.50%   91.52%   +0.02%     
==========================================
  Files         134      134              
  Lines       13472    13497      +25     
==========================================
+ Hits        12327    12353      +26     
+ Misses        727      725       -2     
- Partials      418      419       +1     

Flag	Coverage Δ
unittests	91.52% <88.88%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ReneWerner87]

Reviewed with focus on correctness, concurrency, and RFC semantics. All four fixes are real and correctly implemented: the fixed-window credit guard closes a genuine currHits underflow (rate-limit bypass), the csrf/session Release calls fix session pool leaks without double-release risk, and the new If-None-Match handling matches RFC 9110 §8.8.3.2. Affected suites pass, including with -race.

A few remarks inline. Two notes that do not fit a specific line:

• Behavior changes worth a changelog entry: If-None-Match: * now returns 304 (was 200), a strong client tag now matches a weak server tag, and the ETag header is now also sent on 304 responses. All RFC-correct, but observable.
• Pre-existing, not for this PR: the fixed-window skip path still emits the pre-handler X-RateLimit-Reset value, while the sliding window recomputes it post-handler.

[gaby]

@ReneWerner87 the linter is failing on this one

[gaby]

@ReneWerner87 the linter is failing on this one

Should be good now.