🔥 feat: eliminate global mutex contention across storage-backed middleware (limiter, cache, session)

[pageton]

Problem

A performance architecture review identified that rate limiter, cache, and session middleware all hold a global mutex while performing external storage I/O (Redis, database, etc.). This serializes all requests through these middleware regardless of key, collapsing throughput to ~1/storage-latency.

Affected middleware

Middleware	File	Lock	I/O under lock
Limiter (fixed)	middleware/limiter/limiter_fixed.go:52-91	mux.Lock()	manager.get() + manager.set()
Limiter (sliding)	middleware/limiter/limiter_sliding.go:54-102	mux.Lock()	manager.get() + manager.set()
Cache	middleware/cache/cache.go:313-546	mux.Lock()	storage.Get() / storage.Set()
Session	middleware/session/middleware.go:144-157	m.mu.Lock()	store.Get() → storage.GetWithContext()

Impact

With a 2ms Redis round-trip:

• Effective throughput through any of these middleware caps at ~500 RPS regardless of hardware or concurrency.
• All requests for different keys/IPs serialize on a single mutex.
• At 10K QPS, this becomes the dominant bottleneck.

Proposed fix

Pattern: Per-key striped locking.

1. Replace the single sync.RWMutex with a fixed array of 32 or 64 shards, each with its own mutex.
2. Hash the key (IP, cache key, session ID) to select the shard.
3. Never hold any shard lock across storage I/O — lock, read/write in-memory state, unlock, then perform storage I/O without the lock, then re-lock only to update in-memory state.
4. Optionally keep an in-memory atomic counter as a fast path, syncing to storage asynchronously.

Benchmark suggestion

Before/after with benchstat:

go test -bench=BenchmarkLimiter -benchmem -count=6 ./middleware/limiter/ | tee /tmp/limiter-before.txt
# apply fix
go test -bench=BenchmarkLimiter -benchmem -count=6 ./middleware/limiter/ | tee /tmp/limiter-after.txt
benchstat /tmp/limiter-before.txt /tmp/limiter-after.txt

Priority

P0 — This is the single highest-impact performance issue in the framework. It affects every deployment using external storage with these middleware.

---

Identified during a full performance architecture review of the Fiber codebase.

[ReneWerner87]

Consolidating the per-middleware reports into this umbrella issue. #4326 (session) and #4327 (limiter) were closed in favor of tracking the work here, so the concrete call sites are preserved below.

Session (from #4326) - lock held across storage I/O

• saveSession() - session/session.go:316-340: s.mu.Lock() held while Storage.SetWithContext() runs.
• Regenerate() - session/session.go:221-237: lock held across Storage.DeleteWithContext().
• Reset() - session/session.go:253-274: lock held across Storage.DeleteWithContext().
• Destroy() - session/session.go:187-210: same pattern (RLock variant).

Limiter (from #4327) - single lock across both read and write I/O

• Fixed window - limiter/limiter_fixed.go:52-91: mux.Lock() held across both manager.get() (GetWithContext) and manager.set() (SetWithContext).
• Sliding window - limiter/limiter_sliding.go:54-102: same pattern.

Cache

As described in the original report above.

The proposed direction (striped locking, and never holding a lock across storage I/O: lock, read/write in-memory state, unlock, do the storage I/O, then re-lock only to update in-memory state) applies to all three. PR #4373 is in progress.

[ReneWerner87]

Re-scoping this after closing #4373. The original "striped locking + never hold a lock across I/O" line is one prescription for three middleware that actually have different concurrency semantics, so splitting it:

1. Limiter (fixed + sliding) - needs atomic storage, not striping. get -> check -> increment -> set must be atomic per key, so the lock cannot be released across the I/O (doing so loses increments -> limit bypass). And a per-process lock gives no cross-instance atomicity in the multi-instance Redis setup this issue targets at scale. Correct fix: an optional atomic Increment on the storage path (type-assert fast path + get/set-under-lock fallback for backends without it). Striping is at best a single-instance band-aid and adds per-request allocations (the 11x B/op regression in #4373).
→ Drop the "never hold across I/O" guidance for the limiter.

2. Cache - hold the lock for in-memory heap/accounting only, not across storage I/O. The read path needs no per-key read-modify-write atomicity, so the original guidance applies cleanly here. Storage Get/Set go outside the lock.

3. Session - same as cache. saveSession/Regenerate/Reset/Destroy hold the lock across Storage.*WithContext; narrow to in-memory state, do storage I/O unlocked.

Priority note: in-memory storage (the default) has no slow I/O under the lock, so the throughput cap only applies to external-storage deployments, not "every deployment". Keeping this as the umbrella tracker; the three items above are independent and should land as separate PRs.

[gaby]

@ReneWerner87 Opus wrote the solution in #4373 but its own review doesn't agree on it now?

[ReneWerner87]

@gaby Fair question. It's not the review contradicting itself, it's the analysis going one level deeper than #4373 did.

#4373 applied a single prescription (sharded keyed locking + "never hold a lock across I/O") uniformly across three middleware. The follow-up found those three have genuinely different concurrency requirements, and that this approach is wrong specifically for the limiter:

• The limiter needs get -> check -> increment -> set to be atomic per key. Both get and set are storage I/O, so "never hold the lock across I/O" and "keep the read-modify-write atomic" are mutually exclusive with a process-local lock. Releasing the lock loses increments and lets requests slip past the limit.
• Sharding only serializes within one process. It gives no cross-instance atomicity in the multi-instance Redis setup this issue targets (each instance has its own lock, so two can both read N and write N+1), and in ⚡ perf: reduce lock contention in storage-backed middleware #4373's single-key benchmark the per-request channel allocation showed up as the ~11x B/op regression.
• The correct fix is therefore to push atomicity into storage: an optional Increment capability. The Storage interface deliberately has none today (storage_interface.go), so adding it to the interface would break every existing backend. It has to be an optional method detected via type assertion, with a get/set-under-lock fallback for backends that don't implement it.

For cache and session the original guidance does hold: neither needs per-key read-modify-write (the cache lock only guards the in-memory heap and byte accounting, the session lock is per-instance), so storage I/O can move outside the lock cleanly.

So #4373 wasn't dropped because the idea was bad in general, but because it was the wrong prescription for one of the three. Re-scoping into three independent PRs (limiter = optional atomic Increment, cache + session = narrow the lock) is the result.