🧹 chore: Improve propagation of context.Context #3822
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughContext propagation refactored across storage operations in ctx, cache middleware, and limiter middleware by extracting request-scoped context from Fiber context using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (beta)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3822 +/- ##
==========================================
+ Coverage 92.18% 92.24% +0.05%
==========================================
Files 115 115
Lines 9662 9671 +9
==========================================
+ Hits 8907 8921 +14
+ Misses 484 478 -6
- Partials 271 272 +1
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 removes the unnecessary context.Background() fallback when deriving the request context in limiter and cache middlewares, ensuring that the actual request context is consistently propagated to storage operations.
Key changes:
- Extract request context once using
c.Context()and pass it directly to storage operations - Update test infrastructure to verify proper context propagation
- Apply the same pattern consistently across cache, limiter fixed-window, and limiter sliding-window implementations
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/limiter/limiter_fixed.go | Extracts request context early and passes it to all storage operations |
| middleware/limiter/limiter_sliding.go | Extracts request context early and passes it to all storage operations |
| middleware/limiter/limiter_test.go | Adds test infrastructure and tests to verify context propagation for both limiter types |
| middleware/cache/cache.go | Updates deleteKey and cleanupOnStoreError to accept context parameter; extracts request context early |
| middleware/cache/cache_test.go | Adds test infrastructure and comprehensive tests for context propagation in cache operations |
| ctx.go | Updates SaveFileToStorage to pass request context instead of the Ctx instance |
| ctx_test.go | Adds test to verify context propagation in SaveFileToStorage |
gaby
changed the title
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively improves context.Context propagation throughout the cache and limiter middlewares. The changes correctly replace the use of context.Background() or the fiber.Ctx itself with the actual request context from c.Context(), ensuring that operations like storage access can be properly cancelled. This is a significant improvement for robustness, especially in scenarios involving request cancellation. The introduction of comprehensive tests to verify context propagation, including for edge cases like cache eviction and error cleanup, is commendable and ensures the correctness of these changes. The implementation is consistent across the cache and limiter middlewares.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
middleware/cache/cache_test.go (1)
234-246: Clean context test utilities.The marker-based context creation helpers enable precise verification of context propagation through storage operations.
Note: As flagged in previous reviews, these utilities are duplicated in
limiter_test.go. Consider extracting shared test infrastructure to a common test utilities package (e.g.,internal/testutilor similar) to reduce duplication and ensure consistency across middleware tests.middleware/limiter/limiter_test.go (2)
187-251: Deduplicate verifyRecords helper across tests.The same helper is redefined here and below. Extract it once at package level and reuse.
Add once near the other test helpers:
func verifyRecords(t *testing.T, records []contextRecord, key, wantValue string, wantCanceled bool) { t.Helper() var matched []contextRecord for _, rec := range records { if rec.key == key { matched = append(matched, rec) } } require.Len(t, matched, 2) for _, rec := range matched { require.Equal(t, wantValue, rec.value) require.Equal(t, wantCanceled, rec.canceled) } }Then remove the local duplicates in both tests and call the shared helper.
307-371: Same verifyRecords duplication here; reuse the shared helper.Refactor to call the package-level verifyRecords defined once.
🧹 Nitpick comments (2)
ctx_test.go (2)
3797-3801: Make cancellation observation more robust (avoid 100ms flake).100ms may intermittently fail under CI load. Bump the timeout or poll briefly until ctx.Done() fires.
Apply one of these minimal changes:
Option A — increase timeout:
- case <-time.After(100 * time.Millisecond): + case <-time.After(1 * time.Second):Option B — simple polling (keeps test deterministic without long sleep):
deadline := time.Now().Add(1 * time.Second) for time.Now().Before(deadline) { select { case <-ctx.Done(): s.cancelObserved.Store(true) return nil default: time.Sleep(5 * time.Millisecond) } } s.helperFailure("storage did not observe context cancellation")
3830-3835: Remove unnecessary nil guard in Close.The method is already a no-op; the nil check is redundant.
-func (s *mockContextAwareStorage) Close() error { - if s == nil { - return nil - } - return nil -} +func (s *mockContextAwareStorage) Close() error { return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ctx.go(1 hunks)ctx_test.go(1 hunks)middleware/cache/cache.go(8 hunks)middleware/cache/cache_test.go(2 hunks)middleware/limiter/limiter_fixed.go(3 hunks)middleware/limiter/limiter_sliding.go(3 hunks)middleware/limiter/limiter_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
middleware/limiter/limiter_sliding.gomiddleware/limiter/limiter_fixed.gomiddleware/cache/cache.gomiddleware/cache/cache_test.goctx.gomiddleware/limiter/limiter_test.goctx_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: gaby
PR: gofiber/fiber#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`.
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#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:
middleware/limiter/limiter_sliding.gomiddleware/limiter/limiter_fixed.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
middleware/limiter/limiter_sliding.go
🧬 Code graph analysis (4)
middleware/cache/cache.go (1)
storage_interface.go (1)
Storage(10-46)
middleware/cache/cache_test.go (3)
middleware/cache/cache.go (1)
New(70-374)ctx_interface_gen.go (1)
Ctx(18-426)middleware/cache/config.go (1)
Config(11-80)
middleware/limiter/limiter_test.go (3)
ctx_interface_gen.go (1)
Ctx(18-426)middleware/limiter/limiter_fixed.go (2)
FixedWindow(13-13)FixedWindow(16-133)middleware/limiter/limiter_sliding.go (2)
SlidingWindow(14-14)SlidingWindow(17-164)
ctx_test.go (1)
ctx_interface_gen.go (1)
Ctx(18-426)
⏰ 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). (1)
- GitHub Check: Compare
🔇 Additional comments (15)
ctx.go (1)
444-444: Context propagation implemented correctly.The change properly extracts the request-scoped context using
c.Context()and passes it to the storage operation, ensuring proper context propagation throughout the storage call chain.middleware/cache/cache.go (5)
112-123: Proper context parameterization for deleteKey.The function signature correctly accepts a context parameter, enabling consistent context propagation to both manager and external storage deletion operations.
144-147: Excellent pattern: extract context once and reuse.Extracting the request context at the beginning of the handler and reusing it throughout all storage operations is an efficient and clean approach that ensures consistent context propagation.
167-184: Request context properly propagated through cache hit and expiration paths.Both the expired entry deletion and cached body retrieval correctly use the request context, ensuring consistent context propagation through these critical code paths.
268-268: Eviction path correctly propagates request context.The eviction logic properly uses the request context when removing entries to make room for new cached responses.
325-367: Comprehensive context propagation through storage and cleanup paths.The changes correctly propagate the request context through all storage operations (setRaw, set) and error cleanup paths. The
cleanupOnStoreErrorclosure is properly parameterized to accept context, ensuring consistent context handling across both external storage and in-memory storage paths.middleware/cache/cache_test.go (4)
88-147: Well-designed test infrastructure for context propagation verification.The
contextRecorderStoragewrapper elegantly captures context information across Get/Set/Delete operations, enabling comprehensive verification of context propagation through the cache middleware. The design withcontextRecordFromhelper and safe accessor methods is clean and maintainable.
248-294: Comprehensive eviction context propagation test.The test effectively verifies that when MaxBytes triggers eviction, the delete operations receive the current request's context (not the context from when the evicted entry was stored). This is the correct behavior and the test validates both the context marker and cancellation state.
296-332: Error cleanup path context propagation verified.The test ensures that when storage errors trigger cleanup operations, the request context (including cancellation state) properly propagates to delete operations. This validates that error paths maintain consistent context handling.
334-406: Thorough verification of context propagation through normal cache operations.The test comprehensively validates that both cache miss (Set) and cache hit (Get) paths correctly observe and propagate the request context, including both context markers and cancellation state. The test design with custom headers provides flexible control over context setup.
ctx_test.go (1)
3837-3890: Context propagation test for SaveFileToStorage looks solid.Accurately validates pointer-equal context propagation and cancellation observation through SetWithContext.
middleware/limiter/limiter_sliding.go (1)
46-50: Use of request-scoped context for storage is correct and consistent.Capturing reqCtx := c.Context() once and passing it to manager.get/set ensures proper cancellation/value flow through storage. This matches the intended Context()/RequestCtx() separation. Looks good.
Based on learnings
Also applies to: 107-108, 139-147
middleware/limiter/limiter_fixed.go (1)
45-49: Adopting c.Context() for storage interactions is the right move.The fixed-window limiter now propagates the per-request context to storage uniformly. No issues spotted.
Based on learnings
Also applies to: 76-76, 108-117
middleware/limiter/limiter_test.go (2)
33-81: Context-recording storage wrapper looks good.Captures ctx.Value and canceled state before delegating to underlying storage; copies recorded slices on read. Suitable for these tests.
118-130: Marker helpers are clear and useful.Convenient helpers to tag and pre-cancel contexts for propagation tests.
ReneWerner87
deleted the
update-savefiletostorage-for-context-handling
branch
Summary
Key changes: