fix(server): fix the ephemeral key leak by non-atomic operation#742
fix(server): fix the ephemeral key leak by non-atomic operation#742
Conversation
oxia/options_client.go
Outdated
|
|
||
| // WithSessionKeepAliveTicker is an internal API used to control the duration | ||
| // of the session keep-alive ticker. This is for experimental use only. | ||
| func WithSessionKeepAliveTicker(ticker time.Duration) ClientOption { |
There was a problem hiding this comment.
I wouldn't expose this in API (even if comment says it's internal). We should be able to modify the value by accessing the private variable in the tests
There was a problem hiding this comment.
moved the test into the oxia package and made the WithSessionKeepAliveTicker private.
server/session_manager.go
Outdated
| if _, err := deleteShadow(batch, notification, key, entry); err != nil { | ||
| return err | ||
| } | ||
| if IsSessionKey(key) { |
There was a problem hiding this comment.
nit:
if !IsSessionKey(key) {
return nil;
}
....
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an ephemeral key leak caused by non-atomic operations in session cleanup. The fix ensures that session cleanup operations (including ephemeral key deletion) are performed atomically within the database transaction.
- Refactored session deletion to be atomic by moving ephemeral key cleanup into database callbacks
- Updated all UpdateOperationCallback interfaces to accept notification parameter for proper event handling
- Added comprehensive test coverage for the session key leak scenario
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/session_manager.go | Core fix implementing atomic session cleanup and ephemeral key deletion within DB callbacks |
| server/session.go | Simplified session deletion to rely on atomic DB operations instead of manual cleanup |
| server/secondary_indexes.go | Updated wrapper callbacks to pass notifications parameter through the callback chain |
| server/kv/db.go | Modified UpdateOperationCallback interface to include notifications parameter |
| server/kv/notifications_tracker.go | Made Notifications struct public for use in callback interfaces |
| server/session_manager_test.go | Updated tests to match new callback signatures and added IsSessionKey validation tests |
| server/kv/db_test.go | Updated test callback to match new interface signature |
| oxia/sessions.go | Updated session keep-alive logic to use configurable ticker instead of hardcoded calculation |
| oxia/session_test.go | Added integration test to verify the ephemeral key leak fix |
| oxia/options_client.go | Added internal option for configuring session keep-alive ticker duration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Motivation
A non-atomic operation will leak some ephemeral keys if the new KV is put after the session list.
Modification