Repository navigation
fix(session): bound JWTManager.Terminate cache deletion with a 5s timeout - #924
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR bounds session deletion during termination by adding a ChangesSession Termination Timeout Bounding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/session/jwt.go (1)
129-140: ⚡ Quick wintrim inline rationale comments to terse, timeless wording
Line 129-Line 130 and Line 138-Line 139 are more verbose than needed and include PR-specific history. Keep these comments short and implementation-focused.
As per coding guidelines,
**/*.{go,ts,tsx,js,jsx,py}: Use minimal, DRY, terse comments in lowercase, only when necessary.🤖 Prompt for 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. In `@internal/session/jwt.go` around lines 129 - 140, Trim the verbose inline comments around terminateTimeout and the start of JWTManager.Terminate to concise, implementation-focused notes: replace the multi-line historical rationale on lines around terminateTimeout with a short comment like "// bounds cache deletion in Terminate" and shorten the paragraph before context.WithTimeout to "// derive background context with deadline for cache deletion" (or similar), keeping references to terminateTimeout, JWTManager.Terminate, m.sessionDeleter, and context.WithTimeout so the intent is clear without PR-specific history.
🤖 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 `@internal/session/jwt_test.go`:
- Around line 227-242: Capture the error returned from
manager.Terminate("session-id") instead of discarding it (e.g., assign to err in
the goroutine and send it back to the test), then in the main test goroutine
assert that err matches the expected timeout/deadline behavior (for example
non-nil and context.DeadlineExceeded or contains "deadline exceeded") before or
alongside the existing elapsed-time checks; reference manager.Terminate,
terminateTimeout, deleter.called, and the done channel to locate and wire the
error assertion into the existing selects.
---
Nitpick comments:
In `@internal/session/jwt.go`:
- Around line 129-140: Trim the verbose inline comments around terminateTimeout
and the start of JWTManager.Terminate to concise, implementation-focused notes:
replace the multi-line historical rationale on lines around terminateTimeout
with a short comment like "// bounds cache deletion in Terminate" and shorten
the paragraph before context.WithTimeout to "// derive background context with
deadline for cache deletion" (or similar), keeping references to
terminateTimeout, JWTManager.Terminate, m.sessionDeleter, and
context.WithTimeout so the intent is clear without PR-specific history.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdb86b23-34ad-406e-bb91-96337d366ca2
📒 Files selected for processing (2)
internal/session/jwt.gointernal/session/jwt_test.go
assert errors.Is(err, context.DeadlineExceeded) in the timeout test so the test does not pass on a wrong-error or nil return. trim the inline comments around terminateTimeout and the WithTimeout call to terse, implementation-focused notes per the repo coding guideline (Minimal, DRY, terse comments in lowercase, only when necessary). Signed-off-by: SarthakB11 <[email protected]>
|
Good fix. This will need a minor rebase (typo fix landed on main in the same file). After rebasing, this should be good to go. |
…eout JWTManager.Terminate is called by the upstream MCP server library when the gateway needs to close a session. The SessionIdManager interface does not pass a context, so the implementation creates one internally, but it used context.TODO() which has no cancellation and no deadline. If sessionDeleter is the Redis-backed cache, a network outage means the goroutine driving session expiry blocks indefinitely on Del, leaking the goroutine and potentially holding the upstream library session manager lock. Replace context.TODO() with context.WithTimeout(context.Background(), 5 * time.Second). Mirrors the sessionCloser fix in Kuadrant#911 (Aman-Cool, merged 2026-05-08): same class of bug, same fix shape, symmetric site. Add TestJWTManager_Terminate_Timeout that wires JWTManager with a Deleter mock whose DeleteSessions blocks until ctx is canceled, and asserts Terminate returns within ~6s with a wrapped context.DeadlineExceeded. Signed-off-by: SarthakB11 <[email protected]>
assert errors.Is(err, context.DeadlineExceeded) in the timeout test so the test does not pass on a wrong-error or nil return. trim the inline comments around terminateTimeout and the WithTimeout call to terse, implementation-focused notes per the repo coding guideline (Minimal, DRY, terse comments in lowercase, only when necessary). Signed-off-by: SarthakB11 <[email protected]>
d280115 to
c29a540
Compare
|
Rebased onto main. Picked up the |
|
Hey, thanks for the contribution. Just a heads up: reviews are currently being prioritised around the v0.7 milestone. PRs tied to v0.7 issues will be reviewed first. We'll get to this one, but it may take a bit longer than usual. |
jasonmadigan
left a comment
jasonmadigan
left a comment
There was a problem hiding this comment.
lgtm. bounds the stalled-deleter path correctly, test proves the timeout fires.
…eout (Kuadrant#924) * fix(session): bound JWTManager.Terminate cache deletion with a 5s timeout JWTManager.Terminate is called by the upstream MCP server library when the gateway needs to close a session. The SessionIdManager interface does not pass a context, so the implementation creates one internally, but it used context.TODO() which has no cancellation and no deadline. If sessionDeleter is the Redis-backed cache, a network outage means the goroutine driving session expiry blocks indefinitely on Del, leaking the goroutine and potentially holding the upstream library session manager lock. Replace context.TODO() with context.WithTimeout(context.Background(), 5 * time.Second). Mirrors the sessionCloser fix in Kuadrant#911 (Aman-Cool, merged 2026-05-08): same class of bug, same fix shape, symmetric site. Add TestJWTManager_Terminate_Timeout that wires JWTManager with a Deleter mock whose DeleteSessions blocks until ctx is canceled, and asserts Terminate returns within ~6s with a wrapped context.DeadlineExceeded. Signed-off-by: SarthakB11 <[email protected]> * address coderabbit review on PR Kuadrant#924 assert errors.Is(err, context.DeadlineExceeded) in the timeout test so the test does not pass on a wrong-error or nil return. trim the inline comments around terminateTimeout and the WithTimeout call to terse, implementation-focused notes per the repo coding guideline (Minimal, DRY, terse comments in lowercase, only when necessary). Signed-off-by: SarthakB11 <[email protected]> --------- Signed-off-by: SarthakB11 <[email protected]> Signed-off-by: malladi nagarjuna <[email protected]>
…eout (Kuadrant#924) * fix(session): bound JWTManager.Terminate cache deletion with a 5s timeout JWTManager.Terminate is called by the upstream MCP server library when the gateway needs to close a session. The SessionIdManager interface does not pass a context, so the implementation creates one internally, but it used context.TODO() which has no cancellation and no deadline. If sessionDeleter is the Redis-backed cache, a network outage means the goroutine driving session expiry blocks indefinitely on Del, leaking the goroutine and potentially holding the upstream library session manager lock. Replace context.TODO() with context.WithTimeout(context.Background(), 5 * time.Second). Mirrors the sessionCloser fix in Kuadrant#911 (Aman-Cool, merged 2026-05-08): same class of bug, same fix shape, symmetric site. Add TestJWTManager_Terminate_Timeout that wires JWTManager with a Deleter mock whose DeleteSessions blocks until ctx is canceled, and asserts Terminate returns within ~6s with a wrapped context.DeadlineExceeded. Signed-off-by: SarthakB11 <[email protected]> * address coderabbit review on PR Kuadrant#924 assert errors.Is(err, context.DeadlineExceeded) in the timeout test so the test does not pass on a wrong-error or nil return. trim the inline comments around terminateTimeout and the WithTimeout call to terse, implementation-focused notes per the repo coding guideline (Minimal, DRY, terse comments in lowercase, only when necessary). Signed-off-by: SarthakB11 <[email protected]> --------- Signed-off-by: SarthakB11 <[email protected]>
Summary
Symmetric fix to #911 on the JWT-manager side of session teardown.
JWTManager.Terminateis invoked by the upstream MCP server library when the gateway needs to close a session. TheSessionIdManagerinterface frommark3labs/mcp-godoes not pass acontext.Context, so the implementation creates one internally, but it usedcontext.TODO(). That has no cancellation and no deadline, so ifsessionDeleteris the Redis-backed cache and Redis is unreachable, the goroutine driving session expiry blocks indefinitely on the cache deletion call. That leaks the goroutine and can hold the upstream library's session-manager lock.Replace
context.TODO()withcontext.WithTimeout(context.Background(), terminateTimeout)and defer the cancel.terminateTimeoutis a package-level constant set to 5 seconds so the value is visible at one site and easy to tune. Same shape as #911, just on the other end of the session lifecycle.TestTerminategains a subtest that wiresJWTManagerwith aDeleterwhoseDeleteSessionsblocks until the supplied context is canceled. Without the fix,Terminateblocks forever; with it,Terminatereturns within the deadline plus a small buffer.Verification
Summary by CodeRabbit
Bug Fixes
Tests