Skip to content

Conversation

@gaby
Copy link
Member

@gaby gaby commented Jan 5, 2026

Description

  • Further improvements to cache middleware.

gaby and others added 30 commits December 27, 2025 21:43
…mance

🧹 chore: Optimize cache middleware performance
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>
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>
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 AI and others added 5 commits January 5, 2026 13:31
- Moved sync.RWMutex field to the beginning of failingCacheStorage struct
- This optimizes memory alignment and saves 24 bytes (from 40 to 16 pointer bytes)
- Follows Go best practice of placing larger fields and sync primitives first
- All tests pass

Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Remove early return in refreshHeapIndex for in-memory storage (review 2661538191)
  - In-memory storage also needs manager.set to persist the updated heapidx
  - Previously only modified a local copy which didn't affect the stored entry
- Fix test race condition by using Lock instead of RLock (review 2661538211)
  - Writing to storage.errs map requires exclusive write lock
  - Changed from RLock/RUnlock to Lock/Unlock in deletion failure test
- All tests pass with race detector enabled

Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
fix: Race condition and inconsistent state in cache eviction
Copilot AI review requested due to automatic review settings January 5, 2026 14:08
@gaby gaby requested a review from a team as a code owner January 5, 2026 14:08
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

Warning

Rate limit exceeded

@gaby has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 30 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between de98e43 and 5c044fc.

📒 Files selected for processing (1)
  • middleware/cache/cache.go

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.

Walkthrough

The cache middleware is significantly extended with robust Cache‑Control parsing, per‑request Vary manifest support, MaxBytes reservation and eviction with safe rollback, improved expiration/age calculations (RFC 9111), header/body split storage support, and many time/crypto helper utilities; tests add concurrency and eviction coverage.

Changes

Cohort / File(s) Summary
Core cache middleware
middleware/cache/cache.go
Adds cryptographic helpers (hex buffer pool, hash auth), comprehensive Cache‑Control parsing and request/response directive structs, Vary manifest persist/load/apply, MaxBytes pre‑reservation and eviction logic with heap index tracking and safe rollback, storageExpiration vs in‑memory expiration, RFC‑compliant age/warning handling, header preservation, and date/time utilities.
Cache tests & test storage
middleware/cache/cache_test.go
Makes failingCacheStorage thread‑safe (RWMutex), adds mutating storage test helpers, and expands test coverage for MaxBytes behavior (insufficient space, eviction/delete failures, concurrency), Cache‑Control permutations (s‑maxage, must‑revalidate, proxy‑revalidate, immutable, min‑fresh, max‑stale), Vary/Authorization scenarios, body/key storage, and header/ETag handling.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Middleware as Cache Middleware
    participant MemStore as Memory Store
    participant ExtStore as External Storage

    rect `#F0F8FF`
    Note over Client,Middleware: Request arrival & vary check
    Client->>Middleware: HTTP request (headers)
    Middleware->>MemStore: lookup(meta key + vary manifest)
    MemStore-->>Middleware: hit / miss / partial (body external)
    end

    rect `#FFF0F5`
    Note over Middleware,MemStore: Space reservation & eviction
    alt Need to store new entry
        Middleware->>Middleware: reserve MaxBytes for entry
        alt insufficient space
            Middleware->>MemStore: compute eviction candidates
            Middleware->>MemStore: mark for eviction (release later)
            MemStore->>ExtStore: delete bodies for evicted entries
            ExtStore-->>MemStore: deletion result
            alt eviction failed
                Middleware->>Middleware: restore heap indices & storedBytes
            end
        end
    end
    end

    rect `#F0FFF0`
    Note over Middleware,ExtStore: Store metadata & body
    Middleware->>MemStore: store metadata (headers, age, expiration)
    Middleware->>ExtStore: store body (separate key) if configured
    MemStore-->>Middleware: metadata stored
    ExtStore-->>Middleware: body stored
    end

    Note over Middleware,Client: Serve or revalidate
    Middleware->>Middleware: compute remainingFreshness / RFC9111 age
    alt fresh
        Middleware-->>Client: serve cached response (with warnings if stale)
    else requires revalidation
        Middleware->>Client: forward request to origin / revalidate
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Suggested labels

codex, 📜 RFC Compliance

Suggested reviewers

  • sixcolors
  • ReneWerner87
  • efectn

Poem

🐰 Hops and hashes, bits in a row,

I guard cached carrots where fresh breezes blow.
Evict when it's crowded, restore if it's torn,
Vary and age keep responses reborn.
A tiny rabbit cheers this cache newborn. ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is largely incomplete, providing only a single vague sentence that fails to meet the detailed template requirements for describing changes, features, and impact. Expand the description to include specific changes introduced, affected features (Cache-Control directives, vary support, eviction mechanism), test coverage, and relevant checklist items from the template.
Docstring Coverage ⚠️ Warning Docstring coverage is 29.17% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title is vague and generic, using the non-descriptive term 'Further improvements' that doesn't convey specific information about the substantial cache middleware enhancements. Replace 'Further improvements' with specific details about the main change, such as 'Add cache control directives parsing and eviction mechanism' or similar descriptive phrasing.

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

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

@ReneWerner87 ReneWerner87 added this to v3 Jan 5, 2026
@ReneWerner87 ReneWerner87 added this to the v3 milestone Jan 5, 2026
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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 delivers substantial improvements to the caching middleware, primarily focusing on enhancing its adherence to HTTP caching standards (RFC 9111). The changes introduce more intelligent handling of cache directives, robust support for the Vary header, and a more resilient cache eviction strategy. These updates aim to provide a more reliable, efficient, and standards-compliant caching experience.

Highlights

  • RFC Compliance & Cache-Control Directives: Implemented comprehensive parsing and handling of HTTP Cache-Control and Pragma request directives (e.g., no-cache, no-store, max-age, max-stale, min-fresh, only-if-cached) and response directives (e.g., public, private, s-maxage, must-revalidate, proxy-revalidate) to align with RFC 9111 specifications.
  • Vary Header Support: Introduced robust support for the Vary HTTP header, allowing the cache to store and retrieve different responses based on request header variations. This includes generating unique cache keys and managing Vary manifests.
  • Enhanced Cache Eviction Strategy: Refined the cache eviction mechanism to atomically reserve space for new entries, evict older entries when MaxBytes limit is reached, and include error recovery for failed deletions, ensuring more stable cache management under memory pressure.
  • Improved Cache Entry Structure: The internal item struct for cached entries has been significantly expanded to store more HTTP header information (e.g., Cache-Control, Expires, ETag, Date) and new flags for revalidation and private responses, enhancing the fidelity of cached responses.
  • Optimized Header Storage: Switched from map[string][]byte to a slice of cachedHeader structs for storing response headers within cache entries, which can improve performance and reduce memory allocations during serialization and deserialization.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements significant improvements to the Cache middleware, focusing on RFC 9111 compliance and enhanced cache control handling. The changes add support for sophisticated caching directives including Vary header processing, authorization-based caching, request cache control directives (max-age, max-stale, min-fresh, only-if-cached), response age tracking, and improved cache eviction strategies with better concurrency handling.

Key changes:

  • Enhanced cache control parsing with support for quoted-string values per RFC 9111
  • Authorization header hashing for secure cache separation
  • Vary header support with manifest storage for content negotiation
  • Request cache directives (max-age=0, min-fresh, max-stale, only-if-cached, Pragma: no-cache)
  • Improved MaxBytes eviction with race condition fixes
  • Comprehensive date/age handling with clamping for invalid values

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
middleware/cache/cache.go Core cache logic refactored with new parsing functions, Vary support, authorization handling, improved eviction, and RFC 9111 compliance
middleware/cache/manager.go Item struct expanded with new fields (date, etag, expires, cacheControl, revalidation flags); headers changed from map to slice
middleware/cache/manager_msgp.go Auto-generated msgp serialization code for new item structure and cachedHeader type
middleware/cache/manager_msgp_test.go Auto-generated msgp tests for cachedHeader serialization
middleware/cache/cache_test.go Extensive test coverage with 50+ new test cases covering authorization, Vary, cache control directives, age handling, warnings, and edge cases
ctx_test.go Benchmark configuration updates to add TrustProxy settings for IP-related benchmarks

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
middleware/cache/manager_msgp_test.go (1)

12-33: Consider adding t.Parallel() for test concurrency.

Per coding guidelines, Go tests should invoke t.Parallel() at the start of each test. However, since this is generated code (Code generated by github.com/tinylib/msgp DO NOT EDIT), modifying it directly would be overwritten on regeneration.

Consider either:

  1. Configuring the msgp generator to include t.Parallel() if supported
  2. Accepting this as a limitation of generated code

As per coding guidelines, tests should maximize concurrency with t.Parallel().

middleware/cache/cache.go (2)

291-305: Consider simplifying lock management pattern.

The unlock/relock helper closures with a locked flag add complexity. While this pattern works, it requires careful tracking of lock state throughout the handler.

The pattern is correct but consider whether a more structured approach (e.g., explicit lock scopes with deferred unlocks) would be clearer.


717-720: Redundant non-negative check for Age parsing.

fasthttp.ParseUint returns an int which, when successfully parsed, is always non-negative. The check v >= 0 at line 718 is always true after err == nil.

🔎 Suggested simplification
 		if b := c.Response().Header.Peek(fiber.HeaderAge); len(b) > 0 {
 			if v, err := fasthttp.ParseUint(b); err == nil {
-				if v >= 0 {
-					ageVal = uint64(v)
-				}
+				ageVal = uint64(v)
 			}
 		} else {
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7857454 and b2667b8.

📒 Files selected for processing (6)
  • ctx_test.go
  • middleware/cache/cache.go
  • middleware/cache/cache_test.go
  • middleware/cache/manager.go
  • middleware/cache/manager_msgp.go
  • middleware/cache/manager_msgp_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Prefer github.com/gofiber/utils/v2 helpers (for example, utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code

Files:

  • middleware/cache/manager.go
  • middleware/cache/manager_msgp_test.go
  • ctx_test.go
  • middleware/cache/cache.go
  • middleware/cache/manager_msgp.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

When adding Go tests, always invoke t.Parallel() at the start of each test and subtest to maximize concurrency

Files:

  • middleware/cache/manager_msgp_test.go
  • ctx_test.go
🧠 Learnings (4)
📚 Learning: 2024-06-30T00:38:06.580Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
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/cache/manager_msgp_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.

Applied to files:

  • ctx_test.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.

Applied to files:

  • middleware/cache/cache.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.

Applied to files:

  • middleware/cache/cache.go
🧬 Code graph analysis (1)
ctx_test.go (2)
app.go (2)
  • Config (115-426)
  • TrustProxyConfig (432-456)
constants.go (1)
  • HeaderXForwardedFor (219-219)
🔇 Additional comments (22)
ctx_test.go (2)

2716-2725: Benchmark setup now correctly simulates a trusted loopback proxy for X-Forwarded-For

Using TrustProxyConfig{Loopback: true} together with fastCtx.SetRemoteAddr(127.0.0.1) makes the benchmark accurately reflect a trusted proxy scenario under the current IsProxyTrusted rules, so c.IP() legitimately resolves from X-Forwarded-For. This looks correct and aligned with the newer trust-proxy behavior.


2736-2746: Benchmark with IP validation also correctly configures a trusted loopback source

The EnableIPValidation case mirrors the non-validation benchmark while ensuring the remote address is a loopback IP trusted via TrustProxyConfig{Loopback: true}, which matches the intended behavior of returning the validated forwarded IP. The configuration and context wiring look sound.

middleware/cache/manager.go (3)

17-36: LGTM! Well-structured item type with clear field grouping.

The struct layout groups byte slices together, followed by numeric types, then booleans, which is good for memory alignment. The new fields (cacheControl, expires, etag, date, forceRevalidate, revalidate, private) align well with RFC 9111 cache metadata requirements.


38-41: LGTM! Clean header representation.

The cachedHeader struct provides a simple, serialization-friendly representation for cached HTTP headers.


81-104: LGTM! Complete field reset for pooled items.

All new fields are properly cleared before returning the item to the pool, preventing data leakage between cache entries.

middleware/cache/manager_msgp_test.go (1)

35-123: LGTM! Standard msgp benchmark patterns.

The generated benchmarks follow established msgp conventions with proper ReportAllocs(), ResetTimer(), and SetBytes() calls.

middleware/cache/manager_msgp.go (3)

9-135: LGTM! Generated cachedHeader serialization is correct.

The msgp-generated methods for cachedHeader follow standard patterns with proper error wrapping and unknown field skipping for forward compatibility.


310-507: LGTM! Item encoding correctly handles 17 fields.

The map header size (17) correctly matches the number of fields in the item struct. The nested cachedHeader array serialization properly encodes each header as a key-value map.


750-757: LGTM! Msgsize calculation is comprehensive.

The size calculation correctly accounts for all fields including the new cacheControl, expires, etag, date, and boolean flags, as well as the nested cachedHeader array structure.

middleware/cache/cache.go (13)

43-72: LGTM! Well-structured cache directive types.

The expirationSource enum and requestCacheDirectives struct provide clear, type-safe representations for cache control logic. The boolean flags with corresponding value fields (e.g., maxAge/maxAgeSet) correctly handle optional directives.


74-90: LGTM! Clear documentation for ignored headers.

The inline comments explaining why specific headers are excluded (e.g., "already stored explicitly by the cache manager") improve code maintainability.


146-154: LGTM! Efficient buffer pooling for hex encoding.

The hexBufPool correctly uses *[]byte pointers to avoid allocation on pool Get/Put operations. The pool initialization with proper sizing (hexLen = 64) ensures efficient reuse.


179-200: LGTM! Defensive heap entry removal with thorough bounds checking.

The removeHeapEntry function properly validates:

  1. MaxBytes is enabled
  2. heapIdx is within valid range for indices
  3. indexedIdx is within valid range for entries
  4. Entry key and idx match expected values

This prevents panics from stale or invalid heap indices.


461-493: LGTM! RFC 9111 compliant Age and Date handling.

The implementation correctly:

  • Clamps Date header to prevent future dates (line 470-472)
  • Limits Age to MaxInt32 per RFC 9111 delta-seconds constraints (line 487-488)
  • Adds Warning headers for stale (110) and heuristic (113) responses

611-701: Well-designed atomic reservation with rollback on failure.

The eviction strategy is robust:

  1. Pre-increments storedBytes to atomically reserve space
  2. Uses defer with spaceReserved flag to prevent double-unreserve
  3. On deletion failure, restores sizes, re-adds entries to heap, and refreshes persisted heap indices

This ensures consistent accounting even under failure scenarios.


753-776: LGTM! Correct expiration source priority per RFC 9111.

The expiration calculation correctly prioritizes:

  1. s-maxage for shared caches (when applicable)
  2. max-age directive
  3. Expires header
  4. Configuration default

This aligns with RFC 9111 Section 5.2.2 freshness calculation rules.


947-1007: LGTM! Robust Cache-Control directive parsing with RFC 9111 compliance.

The parseCacheControlDirectives function correctly:

  • Handles leading/trailing whitespace around keys and values
  • Supports quoted-string values with the unquoteCacheDirective helper
  • Processes escaped characters per RFC 9110 Section 5.6.4

1142-1161: LGTM! RFC-compliant age calculation.

The cachedResponseAge function correctly computes the current age as the maximum of:

  • Date-based age (now - date)
  • Resident time (based on exp and ttl)
  • Stored Age header value

This follows RFC 9111 Section 4.2.3 age calculation.


1228-1254: LGTM! Safe time conversions with overflow protection.

The time utility functions handle edge cases correctly:

  • safeUnixSeconds: Returns 0 for pre-epoch times
  • secondsToTime: Clamps to MaxInt64 to prevent int64 overflow
  • secondsToDuration: Prevents Duration overflow by checking against MaxInt64/time.Second

1277-1312: LGTM! Secure Vary key generation with consistent ordering.

The makeBuildVaryKeyFunc implementation:

  • Uses SHA-256 for collision resistance
  • Separates header names and values with null bytes to prevent ambiguity
  • Relies on pre-sorted names (from parseVary) for consistent key generation
  • Efficiently reuses hex encoding buffers via the pool

1339-1351: LGTM! Correct RFC 9111 §3.6 shared cache authorization check.

The allowsSharedCacheDirectives function correctly implements RFC 9111 Section 3.6, which requires explicit shared-cache directives (public, s-maxage, must-revalidate, or proxy-revalidate) to allow caching of responses to authenticated requests. The comment at lines 1347-1350 correctly notes that Expires alone is insufficient.


333-350: Add test coverage for the specific ttl == 0 expiration condition.

While the expiration deletion error case is covered in TestCacheStorageDeleteError (cache_test.go:282-309), there's no explicit test for the condition e.ttl == 0 && e.exp != 0. The lock/unlock pattern is correct—unlocking before the I/O operation, relocking afterward, and unlocking before the goto statement—but this specific code path should have its own test case to ensure the interaction between unlock() (line 334), relock() (line 341), and the final unlock() (line 347) behaves correctly when entries have an expiration timestamp but no TTL.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2667b8d5c

ℹ️ 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".

Comment on lines 807 to 811
if expirationSource != expirationSourceExpires {
if apparentAge > maxAgeSeconds {
ageDuration = expiration + time.Second
} else {
ageDuration = time.Duration(apparentAge) * time.Second

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Deduct Age when Expires drives freshness

When a response supplies an Expires header the middleware skips subtracting the received Age from the freshness lifetime because the age correction is guarded by expirationSource != expirationSourceExpires. As a result remainingExpiration is computed from time.Until(expiresAt) alone, so a response with Expires: 10s but Age: 20 will still be cached and treated as fresh for ~10 seconds even though it is already stale, violating RFC 9111 freshness calculations and risking delivery of expired content.

Useful? React with 👍 / 👎.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces substantial improvements to the cache middleware, focusing on RFC compliance, performance, and maintainability. Key changes include a more robust parsing of Cache-Control headers, support for the Vary header, improved eviction logic to prevent holding locks during I/O, and various performance optimizations like using sync.Pool for buffers. The overall changes make the caching logic more powerful and correct. I've identified a bug in the new heuristic freshness calculation and a few areas for minor refactoring and optimization.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
middleware/cache/cache_test.go (1)

36-44: Critical: Remove duplicate type declaration.

The mutatingStorage type is declared twice (lines 36-39 and 41-44), which will cause a compilation failure. Remove one of the duplicate declarations.

🔎 Proposed fix
 type mutatingStorage struct {
 	data   map[string][]byte
 	mutate func(key string, value []byte) []byte
 }
-
-type mutatingStorage struct {
-	data   map[string][]byte
-	mutate func(key string, value []byte) []byte
-}
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b2667b8 and 21536d4.

📒 Files selected for processing (1)
  • middleware/cache/cache_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

When adding Go tests, always invoke t.Parallel() at the start of each test and subtest to maximize concurrency

Files:

  • middleware/cache/cache_test.go
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Prefer github.com/gofiber/utils/v2 helpers (for example, utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code

Files:

  • middleware/cache/cache_test.go
🧠 Learnings (2)
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.

Applied to files:

  • middleware/cache/cache_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.

Applied to files:

  • middleware/cache/cache_test.go
🧬 Code graph analysis (1)
middleware/cache/cache_test.go (2)
internal/storage/memory/memory.go (1)
  • Storage (16-21)
internal/memory/memory.go (1)
  • Storage (27-30)
🪛 GitHub Check: unit (1.25.x, macos-latest)
middleware/cache/cache_test.go

[failure] 36-36:
other declaration of mutatingStorage

🪛 GitHub Check: unit (1.25.x, ubuntu-latest)
middleware/cache/cache_test.go

[failure] 36-36:
other declaration of mutatingStorage

🪛 GitHub Check: unit (1.25.x, windows-latest)
middleware/cache/cache_test.go

[failure] 36-36:
other declaration of mutatingStorage

⏰ 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). (3)
  • GitHub Check: lint
  • GitHub Check: repeated
  • GitHub Check: Compare
🔇 Additional comments (2)
middleware/cache/cache_test.go (2)

112-168: LGTM! Thread-safe storage implementation.

The addition of sync.RWMutex and proper lock/unlock usage correctly synchronizes access to the shared data and errs maps. Read operations appropriately use RLock while write operations use exclusive Lock.


3798-4834: LGTM! Comprehensive test coverage.

The new test functions provide excellent coverage of edge cases including:

  • MaxBytes eviction and space management
  • Concurrency and race conditions under load
  • Helper function behaviors
  • Cache-Control directive combinations
  • Authorization and Vary header handling

All tests properly invoke t.Parallel() as required by the coding guidelines.

Removed unused 'mutatingStorage' type from cache tests.
@codecov
Copy link

codecov bot commented Jan 5, 2026

Codecov Report

❌ Patch coverage is 79.10448% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.07%. Comparing base (7857454) to head (5c044fc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
middleware/cache/cache.go 79.10% 9 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3989      +/-   ##
==========================================
+ Coverage   90.96%   91.07%   +0.10%     
==========================================
  Files         119      119              
  Lines       10785    10843      +58     
==========================================
+ Hits         9811     9875      +64     
+ Misses        617      612       -5     
+ Partials      357      356       -1     
Flag Coverage Δ
unittests 91.07% <79.10%> (+0.10%) ⬆️

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.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@ReneWerner87 ReneWerner87 merged commit a6c822e into main Jan 5, 2026
13 of 15 checks passed
@ReneWerner87 ReneWerner87 deleted the update-cache-handling-for-authentication branch January 5, 2026 16:37
@github-project-automation github-project-automation bot moved this to Done in v3 Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants