fix: app session update notification#398
Conversation
Summary of ChangesHello @dimast-x, 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 enhances the reliability of app session update notifications by introducing a mechanism to fetch the latest session state from the database before sending notifications. This prevents potential issues where notifications might be sent with stale data. In cases where the database reload fails, the system gracefully falls back to using the recently updated in-memory session. The PR also incorporates routine updates to Go module dependencies. Highlights
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
|
|
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. WalkthroughSubmitAppState now reloads the saved AppSession from the database and uses the reloaded record for notifications/responses; getAppSession omits the status filter when status is empty; notifications now emit Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as AppSessionService
participant DB as Database
participant Notif as Notifier
Client->>Service: SubmitAppState(params)
Service->>DB: apply updates & save (updater.Update)
DB-->>Service: saved (updatedAppSession)
Service->>DB: reload AppSession by ID
alt Reload successful
DB-->>Service: reloaded AppSession
Service->>Notif: emit AppSessionUpdate (rpc.AppSession + rpc.AppAllocation)
Service-->>Client: success (reloaded AppSession)
else Reload fails
DB--xService: reload error
Service-->>Client: RPC error (no fallback)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (3)clearnode/app_session_service.go (1)
clearnode/rpc_router_private.go (1)
clearnode/app_session_service_test.go (4)
⏰ 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). (3)
🔇 Additional comments (6)
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 📢 Thoughts on this report? Let us know! |
![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 aims to fix an issue with app session update notifications by reloading the session from the database after the update transaction. However, the implementation has a bug where the session is reloaded with an incorrect status parameter, causing the reload to always fail and fall back to the previous behavior while logging an error. I've provided a critical comment to fix this. The dependency updates in go.work.sum look standard.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
clearnode/app_session_service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/app_session_service.go (4)
clearnode/log.go (1)
LoggerFromContext(95-100)clearnode/notification.go (2)
NewBalanceNotification(54-61)NewAppSessionNotification(95-122)clearnode/ledger.go (1)
NewAccountID(43-49)clearnode/rpc_router_private.go (1)
AppSessionResponse(62-75)
⏰ 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). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
clearnode/app_session_service.go (1)
482-489: Verify: Using reloaded session may cause notifications with inconsistent state.Following from the race condition concern above, when
appSessioncontains data from a concurrent update (not this operation), the participant allocations retrieved here and the notifications sent will reflect someone else's changes rather than the changes made by this operation.Confirm whether concurrent updates to the same app session are possible in your system. If they are, the current implementation may send incorrect notifications to participants.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
clearnode/app_session.go (1)
34-37: Consider adding a documentation comment for the conditional status filter.The conditional status filtering is a key behavior of this function. Adding a comment would help future maintainers understand that passing an empty status retrieves the latest session regardless of its status.
Apply this diff to add clarifying documentation:
func getAppSession(tx *gorm.DB, sessionID, status string) (*AppSession, error) { var appSession AppSession + // Build query with session_id filter; optionally add status filter if status is non-empty query := tx.Where("session_id = ?", sessionID) if status != "" { query = query.Where("status = ?", status) } if err := query.Order("nonce DESC").First(&appSession).Error; err != nil { return nil, err } return &appSession, nil }clearnode/app_session_service_test.go (3)
440-481: Consider asserting the exact count of AppSessionUpdate notifications and extracting validation to a helper.The current implementation iterates through all notifications and validates each AppSessionUpdate notification found, but doesn't verify that there's exactly one per user. This could mask issues where duplicate notifications are sent.
Consider these improvements:
- Assert exact notification count per user:
// Verify AppSession fields are not empty in the notification + notificationCount := 0 for _, notifications := range capturedNotifications { for _, notification := range notifications { if notification.eventType == AppSessionUpdateEventType { + notificationCount++ notificationData, ok := notification.data.(struct { AppSessionResponse ParticipantAllocations []AppAllocation `json:"participant_allocations"` }) require.True(t, ok, "notification data should be AppSessionUpdateNotification") assert.Equal(t, session.SessionID, notificationData.AppSessionID, "AppSessionID should match") // ... rest of assertions } } } + assert.Equal(t, 2, notificationCount, "Should have exactly one AppSessionUpdate notification per participant")
- Extract to a reusable helper function to avoid duplication across test cases:
func assertAppSessionUpdateNotification(t *testing.T, notification Notification, expectedSessionID string, expectedStatus string, expectedVersion uint64) { notificationData, ok := notification.data.(struct { AppSessionResponse ParticipantAllocations []AppAllocation `json:"participant_allocations"` }) require.True(t, ok, "notification data should be AppSessionUpdateNotification") assert.Equal(t, expectedSessionID, notificationData.AppSessionID) assert.Equal(t, expectedStatus, notificationData.Status) // ... additional assertions }This would improve maintainability and allow adding similar assertions to other test cases like
MultipleParticipantsTokens(lines 490-542).
444-447: Consider defining the notification payload type explicitly rather than using an inline struct.The inline anonymous struct definition works but makes the code harder to maintain and prevents reuse across test cases.
If this type matches the actual notification structure from the service layer, consider importing or defining it at the package level:
// At package level type AppSessionUpdateNotification struct { AppSessionResponse ParticipantAllocations []AppAllocation `json:"participant_allocations"` } // In test notificationData, ok := notification.data.(AppSessionUpdateNotification) require.True(t, ok, "notification data should be AppSessionUpdateNotification")This improves type safety and makes it easier to reuse the validation logic across multiple test cases.
467-467: Timestamp comparison allows equality but message suggests strict inequality.Line 467 uses
updatedAt.After(createdAt) || updatedAt.Equal(createdAt)but the assertion message says "UpdatedAt should be >= CreatedAt", which correctly describes the check. However, Go'stimepackage provides a cleaner alternative.Consider using the more idiomatic approach:
- assert.True(t, updatedAt.After(createdAt) || updatedAt.Equal(createdAt), "UpdatedAt should be >= CreatedAt") + assert.False(t, updatedAt.Before(createdAt), "UpdatedAt should be >= CreatedAt")Or use
assert.GreaterOrEqualfrom testify if available in your version:- assert.True(t, updatedAt.After(createdAt) || updatedAt.Equal(createdAt), "UpdatedAt should be >= CreatedAt") + assert.GreaterOrEqual(t, updatedAt.Unix(), createdAt.Unix(), "UpdatedAt should be >= CreatedAt")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/app_session.go(1 hunks)clearnode/app_session_service_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/app_session_service_test.go (3)
clearnode/notification.go (1)
AppSessionUpdateEventType(46-46)clearnode/rpc_router_private.go (2)
AppSessionResponse(62-75)AppAllocation(56-60)clearnode/pkg/rpc/api.go (4)
AppAllocation(584-591)ChannelStatusOpen(644-644)VersionNitroRPCv0_4(33-33)Version(27-27)
🔇 Additional comments (1)
clearnode/app_session.go (1)
32-42: The review comment can be resolved—all callers already handle the new behavior correctly.The concern about the status filtering change is based on a misunderstanding of the code's intent. ChannelStatus only has three valid values: "open", "closed", and "challenged". Empty string
""is not a valid status.The old implementation (filtering with
status = "") would have returned zero results at line 463, making that code path broken. The new conditional filtering—which skips the status filter when an empty string is passed—actually fixes this bug. Line 463 reloads a session after update and correctly needs to retrieve the session regardless of its status.The three call sites are all correct:
- Line 411 & 507: Pass explicit
"open"status—behavior unchanged- Line 463: Passes
""for a reload after save—now works correctly (was broken before)
2 participants
Summary by CodeRabbit
Bug Fixes
Notifications
API
Tests