🧹 chore: refactor cache handler lock flow

[Rachit-Gandhi]

Summary

• Replace the cache handler's local lock-state closures with a scoped lock guard.
• Remove goto continueRequest by making the cache lookup phase return an explicit handled/error result.
• Deduplicate cached-entry delete/remove flow so storage deletion happens outside the lock and heap/accounting updates happen under the lock.
• Use the same lock helper in the store and eviction sections, and switch the mutex to sync.Mutex because no read-lock paths are used.

Closes #4334.

Validation

Manual validation run locally because the PR checks currently only show labeler/Dependabot jobs:

• make lint -> 0 issues.
• go test ./middleware/cache -race -count=1 -> passed
• go test ./middleware/cache -run Test_Cache_RevalidationUncacheableResponseDeletesStaleEntry -count=1 -> passed
• go test ./middleware/cache -coverprofile=/tmp/fiber-cache-cover.out -count=1 -> passed, coverage: 92.0% of statements
• make test -> DONE 3917 tests, 2 skipped in 54.602s

[welcome]

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[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

Refactors cache middleware locking and request handling, updates invalidation and eviction flows, and adjusts expiration bookkeeping in the cache path.

Changes

Cache lock refactor

Layer / File(s)	Summary
Lock guard primitives and mutex type change middleware/cache/cache.go	Adds cacheLockGuard and withCacheLock, and switches the cache mutex type to *sync.Mutex.
Cache request handling middleware/cache/cache.go, middleware/cache/cache_test.go	Reworks handledCacheRequest, adds deleteCurrentEntry, changes unlock/relock timing around cached bodies and raw-body loading, and adds an only-if-cached auth-entry test.
Post-response invalidation and revalidation middleware/cache/cache.go, middleware/cache/cache_test.go	Routes active-entry invalidation through deleteCurrentEntry(nil, ...), uses deleteRevalidatedEntry for cleanup, and adds tests for private replacement, stale-entry deletion, and concurrent revalidation replacement handling.
Eviction and rollback locking middleware/cache/cache.go	Wraps reservation, eviction candidate handling, rollback, and heap restoration on deletion failure with withCacheLock.
Timestamp and store-path bookkeeping middleware/cache/cache.go	Introduces storeTS, adjusts expiration handling, and wraps heap insertion, cleanup, and old-entry removal with withCacheLock.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• gofiber/fiber#3989: Both change middleware/cache/cache.go around cache-control decisions, authorization/vary handling, and heap/eviction bookkeeping.
• gofiber/fiber#4419: Both modify cache lock and timestamp handling used for freshness decisions.
• gofiber/fiber#3905: Both touch cache-entry shareability and authorization-based caching behavior.

Suggested reviewers: sixcolors, efectn, ReneWerner87

Poem

A bunny hopped through cache and time,
With guarded paws and tidy rhyme.
Old entries poofed, fresh ones shone,
The heap stayed neat, the lock held strong.
🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is useful, but it does not follow the repository's required template or include the requested sections.	Add the required Description template sections, including Fixes #, Changes introduced, Type of change, and the checklist items.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: refactoring the cache handler lock flow.
Linked Issues check	✅ Passed	The refactor matches #4334 by replacing the goto-based lock flow with scoped lock handling and clearer unlock/relock boundaries.
Out of Scope Changes check	✅ Passed	The additional test coverage and cache deletion adjustments are directly related to the lock-flow refactor and cache correctness.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@middleware/cache/cache.go`:
- Around line 346-352: The revalidation flow in cache handling drops e after
releasing the old entry, but later cleanup only runs through deleteCurrentEntry
when e is still non-nil. Update the revalidation paths in cache.go so the code
that handles reqDirectives/maxAge and the response handling for private,
no-cache, and Vary:* preserves whether there was a prior cached entry and its
oldHeapIdx separately from e, then explicitly delete the key and remove the old
heap entry even when e has been cleared. Use the existing
revalidation/deleteCurrentEntry logic as the place to wire in this cleanup in
the affected branches.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d6e26a0f-cf21-4b90-bb8a-c95662184f78

📥 Commits

Reviewing files that changed from the base of the PR and between 05919e5 and a887f89.

📒 Files selected for processing (1)

• middleware/cache/cache.go

[codecov]

Codecov Report

❌ Patch coverage is 74.92877% with 88 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.06%. Comparing base (d1e48af) to head (33ad091).
⚠️ Report is 36 commits behind head on main.

Files with missing lines	Patch %	Lines
middleware/cache/cache.go	74.92%	61 Missing and 27 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4492      +/-   ##
==========================================
- Coverage   93.13%   93.06%   -0.07%     
==========================================
  Files         140      140              
  Lines       14270    14391     +121     
==========================================
+ Hits        13290    13393     +103     
- Misses        611      617       +6     
- Partials      369      381      +12     

Flag	Coverage Δ
unittests	93.06% <74.92%> (-0.07%)	⬇️

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.

[pageton]

Review Summary

Reviewed the full diff, traced every lock acquire/release path, and verified locally:

• ✅ go vet ./middleware/cache/ — clean
• ✅ go test ./middleware/cache/ -race -count=1 — all pass
• ✅ Confirmed zero RLock/RUnlock usage — the RWMutex → Mutex change is safe

No critical issues

The refactoring is well-executed:

• cacheLockGuard with idempotent unlock()/relock() is a strict improvement over the ad-hoc locked/unlock/relock closures.
• IIFE replacing goto continueRequest — clean. ts is correctly scoped inside the closure; the store phase uses storeTS.
• deleteCurrentEntry helper — deduplication is correct. The closure captures e by reference; the order (remove heap entry → release pool entry) is safe since removeHeapEntry reads e.heapidx before release zeroes it.
• withCacheLock in the store/eviction phase — consistent guard + defer pattern throughout.

Bug fix in commit 2 — real correctness improvement

The case revalidate: branch fixes a pre-existing bug in main:

When revalidation sets e = nil (e.g., client sends max-age=0) and the refreshed response comes back private, no-cache, or Vary: *, the old cached entry was orphaned because cleanup was gated solely on e != nil. The stale entry persisted in storage and the heap for its full original TTL, being served as cache hits to subsequent clients despite the origin explicitly marking the response uncacheable.

This is a correctness issue (RFC 9111 violation), not performance or safety:

• Not performance — the stale entry is served as a cache hit (fewer origin calls, not more)
• Not safety — the stale data was already public/cacheable; no new sensitive data is leaked
• Self-healing via TTL expiry, but the stale window can be the full original TTL

Minor notes (non-blocking)

1. Redundant guard: cfg.Storage != nil in deleteCurrentEntry before manager.release(e) is redundant — release() already early-returns when storage == nil. Harmless, just dead defensive code.

2. Test coverage: Patch coverage is ~73% with 60 uncovered lines, concentrated in the new error-handling branches. The revalidation + uncacheable-response fix would especially benefit from a dedicated test — it's a correctness-critical path.

3. CI: No Go test/lint check runs are visible in the PR checks yet (only labeler/Dependabot). Ensure CI gates the merge.

Verdict

Approve. The refactoring is sound, and the included bug fix is a genuine correctness improvement worth merging on its own merit.

[Rachit-Gandhi]

@pageton thanks for the detailed review, I will work through the addressable notes with specific tests for the lines, removal of redundant guard and make ci checks validation as part of the PR summary.

[Copilot]

Pull request overview

Refactors the cache middleware handler’s lock/unlock control flow to remove goto-based paths, centralize lock handling via a guard helper, and simplify deletion/eviction bookkeeping by moving storage I/O outside the lock while keeping heap/accounting updates under lock.

Changes:

• Introduces a cacheLockGuard + withCacheLock helper and switches the handler mutex from sync.RWMutex to sync.Mutex.
• Refactors the cache-lookup phase to return an explicit (handled, error) result instead of using goto continueRequest.
• Deduplicates cached-entry deletion and restructures eviction space reservation/restoration under a single lock helper.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08fcaca7b3

ℹ️ 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f449140ef

ℹ️ 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 26121f8194

ℹ️ 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4c051f84a

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

[ReneWerner87]

Thanks for tackling #4334, the goto/unlock/relock cleanup itself is well executed: the lookup phase as a closure with an explicit (handled, error) result, defer guard.unlock() covering all exit paths, and deleteCurrentEntry deduplicating the three delete flows are clear improvements. The sync.RWMutex to sync.Mutex switch is also correct, there are no read-lock call sites on main. The concurrency tests are carefully built, especially the blocking-storage choreography and the cross-key blocking check.

That said, the PR goes well beyond the refactor the issue asks for, and I think the extra parts need discussion before this can move forward.

1. Hot-path cost without benchmarks

Every cache store (i.e. every miss) now goes through withKeyLock: two extra global mutex acquisitions, a map insert plus delete, and one keyedCacheLock allocation per store, since the lock is removed from the map as soon as refs hits 0. Evictions pay the same per key via deleteLockedKey. This middleware is on a hot path where we have been actively removing single allocations; this change needs before/after numbers for the miss/store path (and ideally the eviction path) to be mergeable.

2. Extra storage round trip on every revalidation

In storage mode, loadRevalidationBody fetches the body snapshot with an additional GET before c.Next() on every revalidation, including the common case where the fresh response turns out to be cacheable and the snapshot is never used. With a remote store (e.g. Redis) that is +1 RTT per revalidation request, and must-revalidate workloads revalidate on every request after expiry.

3. Data race in memory mode

deleteRevalidatedEntry loads current via manager.get, which in memory mode returns the live stored *item pointer, and reads its fields under the key lock only. A concurrent request in the lookup phase mutates the same item under the global mutex (e.exp = ts - 1 in the CacheInvalidator branch). Reader and writer share no lock, so this is a race on item.exp. The existing -race run does not catch it because no test combines memory mode, an invalidator, and a concurrent revalidation.

4. Proportionality of the compare-and-delete machinery

To be fair, this fixes a real bug in the CacheInvalidator case with external storage: e.exp = ts - 1 only mutates the decoded copy, so on main an invalidated must-revalidate entry survives an uncacheable revalidation response with its original expiry and keeps being served as a fresh hit, which your first test demonstrates. For naturally expired must-revalidate entries the harm is smaller (the entry is never served again and expires via the storage TTL). Either way, an unconditional delete in the uncacheable branch fixes the same bug without the snapshot/deep-equal/body-compare/key-lock machinery and without the permanent overhead from points 1 and 2. The machinery only protects against deleting a concurrently stored replacement in a narrow window, and the worst case without it is a single spurious cache miss that the next request repairs. For a cache that seems like the better trade-off.

5. Behavior change in the Authorization path

Previously, an authorized request hitting a non-shareable entry bypassed the cache entirely (return c.Next(), response never stored, entry untouched). Now it calls markRevalidate(), so the fresh response can be stored and replace or delete the existing entry. The blast radius is limited because the key always gets the auth-hash suffix, and the middleware itself only stores shareable entries under auth keys (the store phase bails on hasAuthorization && !isSharedCacheAllowed), so this branch is only reachable with entries seeded by external writers or older versions, as your test does manually. Still, the change is not mentioned in the PR description and the store part is untested (the new test only covers the only-if-cached 504 part, which is a genuine fix and worth keeping).

Minor notes

• sameCachedEntry also compares heapidx; this works because it round-trips through msgp, but deserves a comment. Content-identical replacements remain deletable as an ABA case, which is acceptable for a cache.
• revalidationBodyMatches := cfg.Storage == nil is a misleading name; it really means "body comparison is covered".

Suggestion: split the PR

1. The pure refactor (lock guard, goto removal, mutex switch) matches the issue scope, is easy to review in isolation, and could merge quickly.
2. The only-if-cached 504 fix in the Authorization path, together with its test, as a small separate fix PR.
3. The stale-entry deletion with the key-lock infrastructure needs a design discussion (unconditional delete as the simpler alternative), benchmarks for the store path, and a race test for the memory-plus-invalidator case before it lands.

[ReneWerner87]

#4492 (comment)

[Rachit-Gandhi]

Thanks @ReneWerner87 for the detailed review, I agree the PR balooned beyond the referenced issue, I will split the prs keeping this focused on the refactor. Thanks if it's ok with the contributors I will do it within this week sorry for the late reply