🐛 bug: harden session middleware lifecycle and error handling#4059
🐛 bug: harden session middleware lifecycle and error handling#4059
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Summary of ChangesHello @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 significantly enhances the reliability and resource management of the session middleware. It focuses on preventing session object leaks and application panics by ensuring that session objects are always properly released back to their pool, even when errors occur during initialization or session lifecycle events. The changes centralize session finalization logic and harden error paths within the session store, leading to a more stable and predictable session management system. Highlights
🧠 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. Changelog
Activity
Using Gemini Code AssistThe 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
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 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
|
gaby
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4059 +/- ##
==========================================
+ Coverage 91.04% 91.13% +0.09%
==========================================
Files 119 119
Lines 11340 11370 +30
==========================================
+ Hits 10324 10362 +38
+ Misses 644 638 -6
+ Partials 372 370 -2
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:
|
![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 significantly hardens the session middleware by improving error handling and fixing several potential resource leaks. While the changes are well-implemented, replacing panics with proper error propagation and ensuring session objects are always released back to the pool, a high-severity issue remains. The middleware instance is returned to the pool while a reference to it still exists in the request context's locals, which can lead to cross-request session data leakage if other middlewares access the session after the main handler chain has completed. It is recommended to clear the context local reference before releasing the middleware instance. Additionally, consider improving context propagation during session destruction in middleware/session/store.go.
There was a problem hiding this comment.
Pull request overview
This PR hardens the session middleware/store lifecycle to prevent pooled Session/Middleware objects from leaking across request/error paths, and adds regression tests around those paths.
Changes:
- Convert
(*Middleware).initializeto return an error and route init failures through the configuredErrorHandler/DefaultErrorHandlerinstead of panicking. - Ensure sessions are always released back to the pool on terminal/error paths (
finalizeSession, extraRelease()calls in store error branches). - Add regression tests for initialization, context-local cleanup, and store expiry/destroy behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/session/middleware.go | Switch init to error-return and add finalizeSession() to guarantee session release and locals cleanup. |
| middleware/session/session.go | Add a stored context (gctx) so Destroy() can honor GetByID’s context; clear it on release. |
| middleware/session/store.go | Release sessions on absolute-expiry reset failure; ensure GetByID releases sessions on expiry destroy path and uses gctx. |
| middleware/session/middleware_test.go | Add tests for locals cleanup, init error handling, and finalize lifecycle behavior. |
| middleware/session/store_test.go | Add tests for expiry/reset failure release, expiry destroy release, and destroy context propagation. |
Updated session middleware to store context in Fiber.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: a5953ea | Previous: 92ada8a | Ratio |
|---|---|---|---|
BenchmarkDecoderedirectionMsgs - MB/s |
118.68 MB/s |
47.86 MB/s |
2.48 |
This comment was automatically generated by workflow using github-action-benchmark.
|
…anics Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
🐛 bug: harden session middleware lifecycle and error handling
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly hardens the session middleware by improving its lifecycle management and error handling. Key changes include preventing session object leaks by ensuring sessions are always returned to the pool, handling middleware initialization failures gracefully instead of panicking, and adding robust validation for session IDs as a security measure. The introduction of finalizeSession clarifies the session cleanup logic and fixes a potential resource leak when a session was destroyed. The changes from panicking to returning errors in initialize and acquire* functions make the middleware more robust and align better with Go's error handling practices. The pull request is well-structured and includes a comprehensive set of regression tests that cover the new behaviors and protect against future regressions. Overall, these are excellent improvements that enhance the stability and security of the session middleware.
There was a problem hiding this comment.
Code Review
This pull request significantly hardens the session middleware by improving its lifecycle management and error handling. Key improvements include replacing panics with returned errors in initialization paths, ensuring session objects are consistently released to prevent leaks (especially on error paths), and adding validation for session IDs as a security measure. The introduction of finalizeSession centralizes cleanup logic, making it more robust. The added regression tests are comprehensive and cover the new behaviors well.
I've found a couple of minor opportunities for improvement to make the hardening even more consistent across the codebase. Overall, this is an excellent set of changes that improves the stability and security of the session middleware.
There was a problem hiding this comment.
Code Review
The pull request significantly improves the robustness of the session middleware by ensuring proper lifecycle management and error handling. The transition from panicking to returning errors during initialization, along with the addition of session ID validation and better pooling safety, are excellent improvements. I have a few suggestions to further 'harden' the lifecycle against panics and ensure consistency in context usage across all session operations.
| m := acquireMiddleware() | ||
| m.initialize(c, &cfg) | ||
| if err := m.initialize(c, &cfg); err != nil { | ||
| if cfg.ErrorHandler != nil { | ||
| cfg.ErrorHandler(c, err) | ||
| } else { | ||
| DefaultErrorHandler(c, err) | ||
| } | ||
|
|
||
| releaseMiddleware(m) | ||
| if c.Response().StatusCode() == fiber.StatusOK && len(c.Response().Body()) == 0 { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| stackErr := c.Next() | ||
|
|
||
| m.mu.RLock() | ||
| destroyed := m.destroyed | ||
| m.mu.RUnlock() | ||
|
|
||
| if !destroyed { | ||
| m.saveSession() | ||
| } | ||
| m.finalizeSession() | ||
|
|
||
| fiber.StoreInContext(c, middlewareContextKey, nil) | ||
| releaseMiddleware(m) | ||
| return stackErr |
There was a problem hiding this comment.
To fully 'harden' the middleware lifecycle as intended by this PR, consider using defer for releasing the middleware and finalizing the session. This ensures that pooled objects are returned even if a downstream handler panics, preventing memory leaks in high-load scenarios.
| m := acquireMiddleware() | |
| m.initialize(c, &cfg) | |
| if err := m.initialize(c, &cfg); err != nil { | |
| if cfg.ErrorHandler != nil { | |
| cfg.ErrorHandler(c, err) | |
| } else { | |
| DefaultErrorHandler(c, err) | |
| } | |
| releaseMiddleware(m) | |
| if c.Response().StatusCode() == fiber.StatusOK && len(c.Response().Body()) == 0 { | |
| return err | |
| } | |
| return nil | |
| } | |
| stackErr := c.Next() | |
| m.mu.RLock() | |
| destroyed := m.destroyed | |
| m.mu.RUnlock() | |
| if !destroyed { | |
| m.saveSession() | |
| } | |
| m.finalizeSession() | |
| fiber.StoreInContext(c, middlewareContextKey, nil) | |
| releaseMiddleware(m) | |
| return stackErr | |
| m := acquireMiddleware() | |
| defer releaseMiddleware(m) | |
| if err := m.initialize(c, &cfg); err != nil { | |
| if cfg.ErrorHandler != nil { | |
| cfg.ErrorHandler(c, err) | |
| } else { | |
| DefaultErrorHandler(c, err) | |
| } | |
| if c.Response().StatusCode() == fiber.StatusOK && len(c.Response().Body()) == 0 { | |
| return err | |
| } | |
| return nil | |
| } | |
| defer m.finalizeSession() | |
| defer fiber.StoreInContext(c, middlewareContextKey, nil) | |
| return c.Next() |
| mu sync.RWMutex // Mutex to protect non-data fields | ||
| fresh bool // if new session | ||
| ctx fiber.Ctx // fiber context | ||
| gctx context.Context //nolint:containedctx // Stored to honor GetByID context during Destroy. |
There was a problem hiding this comment.
The addition of gctx to honor the context from GetByID is a good improvement for Destroy. However, for full consistency and correctness, this context should also be honored in other storage-touching methods like Save, Reset, and Regenerate. Currently, those methods fall back to context.Background() if s.ctx is nil, ignoring any timeout or cancellation associated with the context passed to GetByID.
| func (m *Middleware) finalizeSession() { | ||
| m.mu.RLock() | ||
| destroyed := m.destroyed | ||
| m.mu.RUnlock() | ||
|
|
||
| if !destroyed { | ||
| m.saveSession() | ||
| } | ||
|
|
||
| releaseSession(m.Session) | ||
| } |
There was a problem hiding this comment.
Since finalizeSession is now responsible for releasing the session, it should include a nil check for m.Session to prevent potential panics if called on an uninitialized or partially initialized middleware instance (e.g., if used with the defer pattern suggested elsewhere).
func (m *Middleware) finalizeSession() {
if m.Session == nil {
return
}
m.mu.RLock()
destroyed := m.destroyed
m.mu.RUnlock()
if !destroyed {
m.saveSession()
}
releaseSession(m.Session)
}
Motivation
Sessionobjects are always returned to the pool on every terminal/error path.Description
(*Middleware).initializereturnerrorand updateNewWithStoreto invoke the configuredErrorHandlerorDefaultErrorHandleron init failures while releasing the middleware and returning the original error only when no response status was written.finalizeSession()which conditionally callssaveSession()(skips when destroyed) and always callsreleaseSession(m.Session)exactly once to guarantee object lifecycle cleanup.Store.getSessionto callsess.Release()before returning whensess.Reset()fails on absolute-expiry, and changeStore.GetByIDto alwayssess.Release()before returningErrSessionIDNotFoundInStoreafter attemptingsess.Destroy().acquireSession()for balancedRelease()calls and add targeted tests for the new behaviors inmiddleware/session/middleware_test.goandmiddleware/session/store_test.go.