🐛 fix: Heap accounting and data race in cache middleware by Copilot · Pull Request #3985 · gofiber/fiber

Copilot · 2026-01-04T18:24:31Z

Description

This PR addresses two critical issues in the cache middleware that could cause data corruption and race conditions:

Heap accounting during revalidation: When clients force cache revalidation (via Cache-Control: max-age=0, min-fresh, or other directives), the middleware needed to properly manage heap entries to maintain accurate byte accounting for MaxBytes enforcement. The fix ensures that old heap entries remain tracked until a replacement is successfully stored, preventing accounting errors when revalidation fails or the new response isn't cacheable.
Data race in cache entry date mutation: The middleware was mutating the shared cache entry's date field when using in-memory storage. Since the same *item pointer is shared across concurrent requests in memory mode, this mutation caused data races without proper synchronization.

Changes introduced

Heap accounting on revalidation: Modified revalidation handling to defer heap entry removal until after successful storage:
- Track the old heap index when marking for revalidation (client-requested max-age=0, aged-out entries, insufficient remaining freshness, forceRevalidate directive, or expired entries marked for revalidation)
- Keep the old entry in heap/storedBytes accounting throughout the revalidation process
- Add new heap entry before storage attempt
- Remove old heap entry only after storage successfully completes (lines 790-795)
- If storage fails, cleanup removes the new entry while the old entry remains tracked
- This ensures accurate MaxBytes accounting in all scenarios, including storage failures
Thread-safe date handling: Changed from mutating e.date directly to computing the clamped date in a local variable. This prevents data races when using in-memory storage where the same *item pointer is shared across concurrent requests.
Comprehensive test coverage: Added Test_Cache_RevalidationWithMaxBytes with 4 test scenarios:
- max-age=0 revalidation removes old entry on successful storage
- min-fresh revalidation with MaxBytes enabled
- Revalidation respects MaxBytes eviction
- Non-cacheable response preserves old entry
These tests verify heap accounting and storedBytes remain consistent during revalidation with MaxBytes enabled, with proper error handling for all io.ReadAll calls.
Code quality: Applied make generate and make format to ensure code consistency with project standards using gofumpt. Fixed errcheck linter warnings by properly checking error return values in test code.

These changes ensure heap consistency, accurate byte accounting for cache eviction under MaxBytes constraints even during storage failures, and thread-safe operation of the cache middleware.

Benchmarks: No performance impact expected—fixes accounting logic and eliminates unsafe mutation.
Documentation Update: N/A—internal heap management and thread-safety fixes.
Changelog/What's New: Bug fixes for cache eviction byte accounting during revalidation, data race in date handling, and added comprehensive test coverage for revalidation scenarios.
Migration Guide: N/A—transparent fixes.
API Alignment with Express: N/A—internal implementation details.
API Longevity: N/A—no API changes.
Examples: N/A—behavior is now correct as originally intended.

Type of change

Code consistency (non-breaking change which improves code reliability and robustness)

Checklist

Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
Conducted a self-review of the code and provided comments for complex or critical parts.
Updated the documentation in the /docs/ directory for Fiber's documentation.
Added or updated unit tests to validate the effectiveness of the changes or new features.
Ensured that new and existing unit tests pass locally with the changes.
Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
Aimed for optimal performance with minimal allocations in the new code.
Provided benchmarks for the new code to analyze and improve upon.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

coderabbitai · 2026-01-04T18:24:38Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit 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.

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

Remove heap entries before revalidating to prevent duplicate heap records and byte counting issues. This fixes: - maxAge forced revalidation - minFresh forced revalidation - forceRevalidate directive handling - expired entry revalidation Addresses feedback from code review on lines 284-288. Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

gaby · 2026-01-04T18:35:25Z

@copilot Also fix this other issue raised here: #3973 (comment)

Avoid mutating shared cache entry in memory mode by computing clamped date locally instead of modifying e.date. When using in-memory storage, the same *item pointer is shared across concurrent requests, so mutating e.date causes data races. Addresses feedback: #3973 (comment) Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

Copilot · 2026-01-04T18:39:57Z

@copilot Also fix this other issue raised here: #3973 (comment)

Fixed in commit 07e46b9. Changed from mutating e.date directly to computing the clamped date in a local variable clampedDate. This prevents data races when using in-memory storage where the same *item pointer is shared across concurrent requests.

codecov · 2026-01-04T18:52:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.87%. Comparing base (80d999e) to head (33261cb).
⚠️ Report is 9 commits behind head on update-cache-handling-for-authentication.

Additional details and impacted files

@@                            Coverage Diff                            @@
##           update-cache-handling-for-authentication    #3985   +/-   ##
=========================================================================
  Coverage                                     90.86%   90.87%           
=========================================================================
  Files                                           119      119           
  Lines                                         10749    10758    +9     
=========================================================================
+ Hits                                           9767     9776    +9     
  Misses                                          623      623           
  Partials                                        359      359

Flag	Coverage Δ
unittests	`90.87% <100.00%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 07e46b9656

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-01-04T18:58:30Z

middleware/cache/cache.go

 			if reqDirectives.maxAgeSet && (reqDirectives.maxAge == 0 || entryAge > reqDirectives.maxAge) {
 				revalidate = true
+				removeHeapEntry(key, e.heapidx)
 				if cfg.Storage != nil {
 					manager.release(e)


Keep heap accounting when revalidation skips store

When a request forces revalidation (e.g., Cache-Control: max-age=0 or min-fresh), the code now drops the cached item from the heap accounting up front. If the origin response is subsequently not cacheable (no-store, non-cacheable status, Next veto, etc.), the old entry stays in storage but is no longer represented in storedBytes/heap, while the eviction loop that enforces MaxBytes only consults those counters (see the eviction loop around the MaxBytes check). This leaves resident bodies untracked and allows later writes to exceed MaxBytes without eviction. The heap entry should only be removed once a replacement is written or the old entry is actually deleted to keep size accounting correct.

Useful? React with 👍 / 👎.

@copilot See the comment above

Only remove old heap entry when replacement is actually stored. Previously, heap entry was removed immediately when revalidation was triggered, which caused accounting issues if the new response wasn't cacheable. Now we: - Track the old heap index when marking for revalidation - Keep the old entry in heap accounting until replacement - Remove old heap entry only when storing the new entry This ensures MaxBytes accounting remains accurate even when revalidation fails or response is not cacheable. Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

Copilot · 2026-01-04T19:20:10Z

@copilot When a request forces revalidation (e.g., Cache-Control: max-age=0 or min-fresh), the code now drops the cached item from the heap accounting up front. If the origin response is subsequently ...

Fixed in commit 8316bb6. Changed approach to only remove the old heap entry when the replacement is actually stored. Now we:

Track oldHeapIdx when marking for revalidation
Keep the old entry in heap/storedBytes accounting
Remove the old heap entry just before adding the new one (line 740)

This ensures accurate MaxBytes accounting even when revalidation fails or the new response isn't cacheable.

Copilot

Pull request overview

This PR addresses two issues in the cache middleware: heap accounting during revalidation and a data race in cache entry date mutation. The date mutation fix correctly eliminates the race condition, but the heap accounting implementation has a critical bug that contradicts the PR's stated goal.

Key Changes:

Track old heap index (oldHeapIdx) when marking entries for revalidation across multiple code paths (max-age=0, min-fresh, forceRevalidate, and expired entries marked for revalidation)
Change date handling from mutating e.date to using a local clampedDate variable to prevent data races in concurrent access
Attempt to remove old heap entries before storing new ones during revalidation

middleware/cache/cache.go

gaby · 2026-01-04T20:36:56Z

@copilot - The old heap entry is being removed before the storage operation completes successfully. If storage fails (lines 770, 778, or 786), the cleanupOnStoreError function will remove the new heap entry (line 752), leaving no entry in the heap even though the old cached entry should have been preserved. This contradicts the PR description which states "old heap entries remain tracked until a replacement is actually stored."

To fix this properly, the old heap entry should only be removed after the storage operation succeeds. Consider moving the removeHeapEntry call to after the successful storage operations (after line 792), or handle cleanup differently to restore the old entry if storage fails.

Remove old heap entry AFTER successful storage instead of before. This ensures that if storage fails, the old entry remains tracked in the heap. Previously: - Old heap entry was removed before storage - If storage failed, cleanup would remove the new entry - Result: no heap entry at all, but old cached data still in storage Now: - Add new heap entry before storage (as before) - If storage fails, cleanup removes new entry, old stays tracked - If storage succeeds, remove old heap entry after - Result: accurate accounting in all scenarios Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

Added 4 test cases covering: - max-age=0 revalidation removes old entry on success - min-fresh revalidation with MaxBytes - Revalidation respects MaxBytes eviction - Non-cacheable response preserves old entry These tests verify heap accounting remains accurate during revalidation scenarios when MaxBytes is enabled. Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

Copilot · 2026-01-04T20:46:10Z

@copilot - The old heap entry is being removed before the storage operation completes successfully. If storage fails (lines 770, 778, or 786), the cleanupOnStoreError function will remove the new heap...

Fixed in commit be74d15. The old heap entry is now removed only after storage succeeds (lines 790-795), not before. This ensures that if storage fails, the old entry remains tracked in the heap and accurate heap accounting is maintained.

gaby · 2026-01-04T20:53:42Z

@copilot Run the following commands: - make generate - make format

Applied code formatting with gofumpt to ensure consistency with project standards. Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

Copilot · 2026-01-04T20:56:57Z

@copilot Run the following commands: - make generate - make format

Completed in commit 4dc91ca. Ran make generate (no changes) and make format (fixed indentation in test file).

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

gaby · 2026-01-04T21:05:36Z

@copilot Error: middleware/cache/cache_test.go:3453:10: Error return value of io.ReadAll is not checked (errcheck)
body2, _ := io.ReadAll(resp2.Body)
^
Error: middleware/cache/cache_test.go:3461:10: Error return value of io.ReadAll is not checked (errcheck)
body3, _ := io.ReadAll(resp3.Body)
^
Error: middleware/cache/cache_test.go:3495:10: Error return value of io.ReadAll is not checked (errcheck)
body2, _ := io.ReadAll(resp2.Body)
^
Error: middleware/cache/cache_test.go:3503:10: Error return value of io.ReadAll is not checked (errcheck)
body3, _ := io.ReadAll(resp3.Body)
^
Error: middleware/cache/cache_test.go:3589:10: Error return value of io.ReadAll is not checked (errcheck)
body1, _ := io.ReadAll(resp1.Body)
^
Error: middleware/cache/cache_test.go:3598:10: Error return value of io.ReadAll is not checked (errcheck)
body2, _ := io.ReadAll(resp2.Body)
^
Error: middleware/cache/cache_test.go:3607:10: Error return value of io.ReadAll is not checked (errcheck)
body3, _ := io.ReadAll(resp3.Body)

Check error return values from io.ReadAll in test cases to satisfy errcheck linter. Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

Copilot · 2026-01-04T21:09:56Z

@copilot Error: middleware/cache/cache_test.go:3453:10: Error return value of io.ReadAll is not checked (errcheck)
body2, _ := io.ReadAll(resp2.Body)
^...

Fixed in commit 33261cb. Added proper error checking for all io.ReadAll calls using require.NoError(t, err) to satisfy errcheck linter.

Initial plan

ce8d502

Copilot AI assigned Copilot and gaby Jan 4, 2026

Copilot AI mentioned this pull request Jan 4, 2026

🧹 chore: Improve Cache middleware compliance #3973

Merged

Copilot started work on behalf of gaby January 4, 2026 18:24 View session

Copilot AI changed the title ~~[WIP] Address feedback on Cache middleware compliance improvements~~ Fix heap entry cleanup during cache revalidation Jan 4, 2026

Copilot AI requested a review from gaby January 4, 2026 18:32

Copilot finished work on behalf of gaby January 4, 2026 18:32

Copilot started work on behalf of gaby January 4, 2026 18:35 View session

Copilot AI changed the title ~~Fix heap entry cleanup during cache revalidation~~ Fix heap entry cleanup and data race in cache middleware Jan 4, 2026

Copilot finished work on behalf of gaby January 4, 2026 18:40

ReneWerner87 added the ☢️ Bug label Jan 4, 2026

gaby changed the title ~~Fix heap entry cleanup and data race in cache middleware~~ fix: Heap entry cleanup and data race in cache middleware Jan 4, 2026

gaby marked this pull request as ready for review January 4, 2026 18:52

gaby requested a review from a team as a code owner January 4, 2026 18:52

gaby requested review from ReneWerner87, efectn and sixcolors and removed request for a team January 4, 2026 18:52

gaby added the v3 label Jan 4, 2026

ReneWerner87 added this to the v3 milestone Jan 4, 2026

ReneWerner87 added this to v3 Jan 4, 2026

chatgpt-codex-connector bot reviewed Jan 4, 2026

View reviewed changes

Copilot started work on behalf of gaby January 4, 2026 19:03 View session

Copilot started work on behalf of gaby January 4, 2026 19:14 View session

Copilot AI changed the title ~~🐛 fix: Heap entry cleanup and data race in cache middleware~~ 🐛 fix: Heap accounting and data race in cache middleware Jan 4, 2026

Copilot finished work on behalf of gaby January 4, 2026 19:20

gaby requested a review from Copilot January 4, 2026 20:00

Copilot started reviewing on behalf of gaby January 4, 2026 20:01 View session

Copilot AI reviewed Jan 4, 2026

View reviewed changes

middleware/cache/cache.go Outdated Show resolved Hide resolved

middleware/cache/cache.go Show resolved Hide resolved

Copilot started work on behalf of gaby January 4, 2026 20:37 View session

Copilot AI and others added 2 commits January 4, 2026 20:39

Copilot finished work on behalf of gaby January 4, 2026 20:47

Copilot started work on behalf of gaby January 4, 2026 20:54 View session

Run make generate and make format

4dc91ca

Applied code formatting with gofumpt to ensure consistency with project standards. Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

Copilot finished work on behalf of gaby January 4, 2026 20:57

gaby requested a review from Copilot January 4, 2026 21:00

Copilot started reviewing on behalf of gaby January 4, 2026 21:00 View session

Copilot AI reviewed Jan 4, 2026

View reviewed changes

Copilot started work on behalf of gaby January 4, 2026 21:06 View session

Fix errcheck linter warnings in test file

33261cb

Check error return values from io.ReadAll in test cases to satisfy errcheck linter. Co-authored-by: gaby <835733+gaby@users.noreply.github.com>

Copilot finished work on behalf of gaby January 4, 2026 21:10

gaby merged commit 1e57098 into update-cache-handling-for-authentication Jan 4, 2026
17 checks passed

gaby deleted the copilot/sub-pr-3973 branch January 4, 2026 21:20

github-project-automation bot moved this to Done in v3 Jan 4, 2026

Uh oh!

Conversation

Copilot AI commented Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes introduced

Type of change

Checklist

Uh oh!

coderabbitai bot commented Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Other AI code review bot(s) detected

Uh oh!

gaby commented Jan 4, 2026

Uh oh!

Copilot AI commented Jan 4, 2026

Uh oh!

codecov bot commented Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

gaby Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Jan 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

gaby commented Jan 4, 2026

Uh oh!

Copilot AI commented Jan 4, 2026

Uh oh!

gaby commented Jan 4, 2026

Uh oh!

Copilot AI commented Jan 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

gaby commented Jan 4, 2026

Uh oh!

Copilot AI commented Jan 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Jan 4, 2026 •

edited

Loading

coderabbitai bot commented Jan 4, 2026 •

edited

Loading

codecov bot commented Jan 4, 2026 •

edited

Loading