🧹 chore: Improve error handling when using storage drivers #3754
Conversation
WalkthroughThis PR adds explicit error returns and propagation across cache, CSRF, and limiter middleware. Manager methods now return errors, callers handle them, and tests introduce failing storage stubs to assert HTTP error paths. Several msgp-generated methods switch to pointer receivers. Control flow adds early returns and consistent unlocking on failures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant MW as Middleware
participant Mgr as Manager
participant Store as Storage
rect rgb(245,248,252)
note over MW,Mgr: Read path with error-aware cache/CSRF/limiter managers
Client->>MW: Request
MW->>Mgr: get/getRaw(key)
alt storage-backed
Mgr->>Store: GetWithContext(key)
Store-->>Mgr: raw or error
alt error
Mgr-->>MW: error
MW-->>Client: Error via ErrorHandler
else miss/ok
Mgr-->>MW: item/raw
MW->>MW: proceed
end
else memory-backed
Mgr->>Mgr: type-assert/lookup
alt type mismatch
Mgr-->>MW: error
MW-->>Client: Error via ErrorHandler
else ok
Mgr-->>MW: item/raw
end
end
end
rect rgb(237,251,245)
note over MW,Mgr: Write path with error propagation
MW->>Mgr: set/setRaw(key, val, exp)
alt storage-backed
Mgr->>Mgr: MarshalMsg
alt marshal error
Mgr-->>MW: error
MW-->>Client: Error via ErrorHandler
else ok
Mgr->>Store: SetWithContext(key, bytes, exp)
Store-->>Mgr: ok or error
alt error
Mgr-->>MW: error
MW-->>Client: Error via ErrorHandler
else ok
Mgr-->>MW: success
MW->>Client: Continue
end
end
else memory-backed
Mgr-->>MW: success
end
end
sequenceDiagram
autonumber
actor Client
participant CSRF as CSRF MW
participant SM as storageManager
participant Store as Storage
Client->>CSRF: Request with token
CSRF->>SM: getRaw(tokenKey)
SM->>Store: GetWithContext
Store-->>SM: raw or error
alt error
CSRF-->>Client: Routed to ErrorHandler
else ok
alt SingleUseToken
CSRF->>SM: delRaw(tokenKey)
SM->>Store: DeleteWithContext
Store-->>SM: ok or error
alt error
CSRF-->>Client: Routed to ErrorHandler
else ok
CSRF-->>Client: Next
end
else reuse
CSRF->>SM: setRaw(tokenKey,new,exp)
SM->>Store: SetWithContext
Store-->>SM: ok or error
alt error
CSRF-->>Client: Routed to ErrorHandler
else ok
CSRF-->>Client: Next
end
end
end
sequenceDiagram
autonumber
actor Client
participant Lim as Limiter MW
participant Mgr as Manager
participant Store as Storage
Client->>Lim: Request
Lim->>Mgr: get(key)
Mgr->>Store: GetWithContext
Store-->>Mgr: raw or error
alt error
Lim-->>Client: Error (wrapped)
else ok/miss
Lim->>Lim: update counters
Lim->>Mgr: set(key, item, exp)
Mgr->>Store: SetWithContext
Store-->>Mgr: ok or error
alt error
Lim-->>Client: Error "limiter: failed to persist state"
else success
Lim-->>Client: 200 or 429
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate 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. Comment |
gaby
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3754 +/- ##
==========================================
- Coverage 92.04% 91.44% -0.60%
==========================================
Files 114 113 -1
Lines 11799 11806 +7
==========================================
- Hits 10860 10796 -64
- Misses 680 743 +63
- Partials 259 267 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling when using storage drivers in the CSRF, limiter, and cache middleware. The changes add proper error propagation and handling when storage operations fail, replacing previously ignored errors with appropriate error messages.
Key changes:
- Enhanced error handling for storage operations (get, set, delete) across middleware
- Converted void functions to return errors for better error propagation
- Added comprehensive test coverage for storage failure scenarios
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/session/data_msgp.go | Updates method receivers from value to pointer for msgp generated code |
| middleware/limiter/manager.go | Adds error handling and type checking for storage and memory operations |
| middleware/limiter/limiter_test.go | Adds test cases for storage get and set error scenarios |
| middleware/limiter/limiter_sliding.go | Updates to handle storage errors and propagate them properly |
| middleware/limiter/limiter_fixed.go | Updates to handle storage errors and propagate them properly |
| middleware/csrf/storage_manager_msgp.go | Simplifies return statements in msgp generated code |
| middleware/csrf/storage_manager.go | Adds error handling for storage operations and type checking |
| middleware/csrf/csrf_test.go | Adds comprehensive test coverage for storage error scenarios |
| middleware/csrf/csrf.go | Updates CSRF handlers to properly handle and propagate storage errors |
| middleware/cache/manager_test.go | Updates tests to handle new error returns from manager methods |
| middleware/cache/manager.go | Adds error handling for all storage operations and introduces cache miss error |
| middleware/cache/cache_test.go | Adds test cases for cache storage error scenarios |
| middleware/cache/cache.go | Updates cache handlers to properly handle storage errors |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
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, or 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 fix this CI failure" or "@codex address that feedback".
|
|
||
| // For external Storage we store raw body separated | ||
| if cfg.Storage != nil { | ||
| manager.setRaw(c, key+"_body", e.body, expiration) | ||
| if err := manager.setRaw(c, key+"_body", e.body, expiration); err != nil { | ||
| manager.release(e) | ||
| return err | ||
| } | ||
| // avoid body msgp encoding | ||
| e.body = nil | ||
| manager.set(c, key, e, expiration) | ||
| if err := manager.set(c, key, e, expiration); err != nil { | ||
| manager.release(e) | ||
| return err | ||
| } |
There was a problem hiding this comment.
[P1] Roll back heap accounting on storage write errors
When a storage write fails in New, the code returns immediately after manager.setRaw/manager.set but the entry has already been inserted into the LRU heap and storedBytes was incremented before the failing call. This leaves the heap containing a stale item and keeps storedBytes inflated even though nothing was cached, so subsequent requests may believe the cache is full and evict entries or refuse new ones until another eviction happens. Consider removing the heap entry and subtracting its size before returning an error to keep cache accounting consistent.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/cache/manager.go (1)
64-77: Reset all fields on release to prevent stale data.
cencodingandheapidxaren’t reset, risking leakage across pooled uses.func (m *manager) release(e *item) { // don't release item if we using in-memory storage if m.storage == nil { return } e.body = nil e.ctype = nil + e.cencoding = nil e.status = 0 e.age = 0 e.exp = 0 e.ttl = 0 e.headers = nil + e.heapidx = 0 m.pool.Put(e) }
🧹 Nitpick comments (16)
middleware/limiter/limiter_test.go (1)
19-66: Make the failing storage stub goroutine-safe.Tests run in parallel and Fiber may process requests concurrently; guard the maps with a mutex to avoid future racy reads/writes if these helpers get reused in other tests.
Apply:
type failingLimiterStorage struct { - data map[string][]byte - errs map[string]error + mu sync.RWMutex + data map[string][]byte + errs map[string]error } func newFailingLimiterStorage() *failingLimiterStorage { return &failingLimiterStorage{ - data: make(map[string][]byte), - errs: make(map[string]error), + data: make(map[string][]byte), + errs: make(map[string]error), } } func (s *failingLimiterStorage) GetWithContext(_ context.Context, key string) ([]byte, error) { - if err, ok := s.errs["get|"+key]; ok && err != nil { + s.mu.RLock() + err, ok := s.errs["get|"+key] + s.mu.RUnlock() + if ok && err != nil { return nil, err } - if val, ok := s.data[key]; ok { + s.mu.RLock() + val, ok := s.data[key] + s.mu.RUnlock() + if ok { return append([]byte(nil), val...), nil } return nil, nil } func (s *failingLimiterStorage) SetWithContext(_ context.Context, key string, val []byte, _ time.Duration) error { - if err, ok := s.errs["set|"+key]; ok && err != nil { + s.mu.RLock() + err, ok := s.errs["set|"+key] + s.mu.RUnlock() + if ok && err != nil { return err } - s.data[key] = append([]byte(nil), val...) + s.mu.Lock() + s.data[key] = append([]byte(nil), val...) + s.mu.Unlock() return nil }middleware/limiter/limiter_sliding.go (3)
46-51: Pass cancellable context to storage calls (honor timeout middleware).Fiber’s Ctx implements context.Context but Deadline/Done are no-ops; timeout middleware exposes the real derived context via c.Context(). Use it so store drivers can observe cancellations/timeouts.
- e, err := manager.get(c, key) + e, err := manager.get(c.Context(), key)
104-107: Propagate cancellation to Set as well.- if setErr := manager.set(c, key, e, time.Duration(resetInSec+expiration)*time.Second); setErr != nil { //nolint:gosec // Not a concern + if setErr := manager.set(c.Context(), key, e, time.Duration(resetInSec+expiration)*time.Second); setErr != nil { //nolint:gosec // Not a concern
136-147: Use c.Context() in Skip branch for get/set to keep semantics consistent.*- entry, getErr := manager.get(c, key) + entry, getErr := manager.get(c.Context(), key) if getErr != nil { mux.Unlock() return getErr } e = entry e.currHits-- remaining++ - if setErr := manager.set(c, key, e, cfg.Expiration); setErr != nil { + if setErr := manager.set(c.Context(), key, e, cfg.Expiration); setErr != nil { mux.Unlock() return fmt.Errorf("limiter: failed to persist state: %w", setErr) }middleware/limiter/limiter_fixed.go (3)
45-50: Pass the derived context into storage get.Aligns with timeout middleware and cancellation propagation.
- e, err := manager.get(c, key) + e, err := manager.get(c.Context(), key)
73-76: Pass the derived context into storage set.- if setErr := manager.set(c, key, e, cfg.Expiration); setErr != nil { + if setErr := manager.set(c.Context(), key, e, cfg.Expiration); setErr != nil { mux.Unlock() return fmt.Errorf("limiter: failed to persist state: %w", setErr) }
105-116: Keep Skip branch consistent with context propagation.*- entry, getErr := manager.get(c, key) + entry, getErr := manager.get(c.Context(), key) if getErr != nil { mux.Unlock() return getErr } e = entry e.currHits-- remaining++ - if setErr := manager.set(c, key, e, cfg.Expiration); setErr != nil { + if setErr := manager.set(c.Context(), key, e, cfg.Expiration); setErr != nil { mux.Unlock() return fmt.Errorf("limiter: failed to persist state: %w", setErr) }middleware/limiter/manager.go (2)
42-44: Tiny typo in comment.“too” → “to”.
- // Fallback too memory storage + // Fallback to memory storage
62-67: Avoid logging/returning raw keys (may contain PII like IPs).Including the full key in error strings can leak user IPs or custom identifiers to logs. Consider redacting or truncating in the formatted message.
- return nil, fmt.Errorf("limiter: failed to get key %q from storage: %w", key, err) + return nil, fmt.Errorf("limiter: failed to get key from storage: %w", err)Similarly adjust other error formats that embed %q.
middleware/csrf/csrf.go (1)
109-113: Optional: consider fail‑open for safe methods on transient storage errors.For GET/HEAD/OPTIONS/TRACE with a cookie present, you might opt to ignore storage errors (treat as token missing) instead of failing the request, to improve availability. Gate behind a config flag if needed.
middleware/csrf/csrf_test.go (1)
20-81: Great failing storage test double; add a regression for secret redaction.Nice targeted error‑path coverage. After removing key from errors, assert the token isn’t present in captured error messages.
require.ErrorContains(t, captured, "csrf: failed to fetch token from storage") +require.NotContains(t, captured.Error(), "token")Apply the same NotContains in TestCSRFStorageSetError and TestCSRFStorageDeleteError.
Also applies to: 82-176
middleware/cache/manager_test.go (2)
18-21: Prefer require over assert for nil checks in fatal paths.
Switchassert.Nil(t, it)torequire.Nil(t, it)so the subtest stops immediately on failure.- assert.Nil(t, it) + require.Nil(t, it)
27-30: Strengthen positive-path assertions.
Also assert the stored body equals what was set to catch serialization/retention regressions.it, err := cacheManager.get(context.Background(), id) require.NoError(t, err) assert.NotNil(t, it) + require.Equal(t, []byte("test-body"), it.body)middleware/cache/cache_test.go (1)
88-171: Consider adding a missing-body scenario test.
Prepopulate metadata key only (no/_GET_body) and assertcacheBodyFetchErroris returned on hit.middleware/cache/cache.go (1)
103-114: Use request context for storage deletes.
deleteKeyusescontext.Background(), which ignores cancellations/timeouts. Thread through the request context for better control.- deleteKey := func(dkey string) error { - if err := manager.del(context.Background(), dkey); err != nil { + deleteKey := func(ctx context.Context, dkey string) error { + if err := manager.del(ctx, dkey); err != nil { return err } // External storage saves body data with different key if cfg.Storage != nil { - if err := manager.del(context.Background(), dkey+"_body"); err != nil { + if err := manager.del(ctx, dkey+"_body"); err != nil { return err } } return nil }And update call sites:
- if err := deleteKey(key); err != nil { + if err := deleteKey(c, key); err != nil {- keyToRemove, size := heap.removeFirst() - if err := deleteKey(keyToRemove); err != nil { + keyToRemove, size := heap.removeFirst() + if err := deleteKey(c, keyToRemove); err != nil {middleware/cache/manager.go (1)
17-28: Consider excluding heap index from serialization.
Persistingheapidxcouples on-disk data to an in-process structure. If feasible, mark it ignored (e.g.,heapidx int \msg:"-"``) and derive indices from the heap map instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
middleware/cache/cache.go(8 hunks)middleware/cache/cache_test.go(2 hunks)middleware/cache/manager.go(3 hunks)middleware/cache/manager_test.go(1 hunks)middleware/csrf/csrf.go(7 hunks)middleware/csrf/csrf_test.go(3 hunks)middleware/csrf/storage_manager.go(2 hunks)middleware/csrf/storage_manager_msgp.go(6 hunks)middleware/limiter/limiter_fixed.go(5 hunks)middleware/limiter/limiter_sliding.go(5 hunks)middleware/limiter/limiter_test.go(2 hunks)middleware/limiter/manager.go(2 hunks)middleware/session/data_msgp.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/limiter/limiter_sliding.gomiddleware/cache/manager_test.gomiddleware/csrf/storage_manager.gomiddleware/limiter/manager.gomiddleware/limiter/limiter_fixed.gomiddleware/limiter/limiter_test.gomiddleware/cache/cache.gomiddleware/csrf/csrf.gomiddleware/cache/manager.gomiddleware/session/data_msgp.gomiddleware/cache/cache_test.gomiddleware/csrf/csrf_test.gomiddleware/csrf/storage_manager_msgp.go
🧠 Learnings (5)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/cache/manager_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/cache/manager_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/cache/manager_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
middleware/csrf/storage_manager.gomiddleware/limiter/manager.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Applied to files:
middleware/csrf/csrf_test.go
🧬 Code graph analysis (6)
middleware/limiter/limiter_test.go (2)
internal/storage/memory/memory.go (2)
New(28-44)Storage(14-19)middleware/limiter/config.go (1)
Config(10-70)
middleware/cache/cache.go (1)
storage_interface.go (1)
Storage(10-46)
middleware/csrf/csrf.go (2)
app.go (2)
ErrorHandler(59-59)Config(113-418)middleware/csrf/config.go (1)
Config(16-121)
middleware/cache/manager.go (1)
middleware/cache/cache.go (1)
New(70-335)
middleware/cache/cache_test.go (1)
middleware/cache/config.go (1)
Config(11-75)
middleware/csrf/csrf_test.go (3)
middleware/csrf/csrf.go (1)
New(47-205)middleware/csrf/config.go (3)
Config(16-121)ConfigDefault(127-134)HeaderName(124-124)storage_interface.go (1)
Storage(10-46)
⏰ 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). (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (26)
middleware/limiter/limiter_test.go (2)
67-91: LGTM: Get error path is asserted end-to-end.Good capture via ErrorHandler and substring checks; this validates the wrapped message without over-coupling.
93-118: LGTM — assertions are precise; approving.rg confirms the messages originate from middleware/limiter/manager.go ("limiter: failed to store key") and from middleware/limiter/limiter_fixed.go and middleware/limiter/limiter_sliding.go ("limiter: failed to persist state"); only middleware/limiter/limiter_test.go asserts those strings.
middleware/limiter/manager.go (2)
62-90: Solid: clear error surfaces and pool hygiene.Good wrapping on storage errors, release on unmarshal failure, and strict type checks for memory path.
Optional: ensure there’s a test that forces an unmarshal error (e.g., store invalid bytes) to cover the new branch.
93-110: LGTM: release semantics on all storage set paths.Releasing on both success and error avoids pool leaks; memory path correctly keeps the pointer owned by the store.
middleware/session/data_msgp.go (1)
39-47: Pointer receivers for msgp methods: good; ensure no value-typed call sites remain.The switch to pointer receivers for EncodeMsg/MarshalMsg/Msgsize is fine and matches other msgp updates in this PR. Please confirm all internal uses take a *data, not a value, to avoid method-set mismatches.
Also applies to: 50-56, 89-92
middleware/csrf/csrf.go (2)
109-113: Error propagation to ErrorHandler: solid and consistent.The new error paths for get/store/delete correctly funnel to cfg.ErrorHandler and align with the updated storage manager API.
Also applies to: 161-165, 174-176, 189-191, 299-301
229-238: Double‑wrap wording is fine; verify no secret leakage.getRawFromStorage wraps underlying storage errors with context. Once storage_manager stops embedding the token (see sibling comment), messages won’t leak secrets.
middleware/csrf/csrf_test.go (1)
1738-1749: Session and storage delete tests: good coverage.Validates both session-backed and storage-backed delete paths with the new error-aware APIs.
Also applies to: 1745-1750
middleware/csrf/storage_manager_msgp.go (1)
124-132: Pointer receivers for storageManager msgp methods: OK.Matches runtime usage where storageManager is held as a pointer. Regenerate msgp artifacts consistently to avoid drift.
Also applies to: 135-141, 174-177
middleware/cache/manager_test.go (1)
10-10: Good addition: use require for fatal assertions.middleware/cache/cache_test.go (9)
7-8: New imports are appropriate for error‑injection tests.
26-36: Test storage stub looks correct and mirrors fiber.Storage semantics.
Clones byte slices to avoid aliasing and returns(nil, nil)on miss as required.
38-47: LGTM: GetWithContext mimics per‑key failures via errs map.
52-59: LGTM: SetWithContext handles injected errors and copies payloads.
64-70: LGTM: DeleteWithContext returns nil on missing keys, as required.
76-86: Reset/Close helpers are fine.
88-112: Great coverage of get‑path failures.
Captures and asserts the propagated error and 500 response.
114-138: Great coverage of set‑path failures.
Validates raw‑body store error propagation.
140-171: Great coverage of delete‑path failures on expiry.
Seeds an expired entry and asserts wrapped delete error reaches ErrorHandler.middleware/cache/cache.go (4)
136-140: Confirm: does fiber.Ctx implement context.Context?
manager.get(c, key)(and friends) require acontext.Context. Iffiber.Ctxdoes not implement it in v3, these calls won’t compile; otherwise, all good. Please confirm.
171-178: Good: body fetch error path releases and unlocks before returning.
255-257: Good: eviction path now propagates delete errors with key context.
360-366: Helpful sentinel translation for missing raw bodies.
Clearer error when metadata exists but body is missing.middleware/cache/manager.go (3)
37-38: Sentinel miss is appropriate and used with errors.Is.
80-109: Storage get wrapping is solid; caller must release on success.
Ensure all call sites release the item on both success and all early returns; see cache.go expiry path fix.
135-152: set (storage mode) owns release — document and keep call sites release-free.
Matches the fix proposed in cache.go to avoid double Put.
Summary
Replaces #3597
Fixes #3020