Harden Session I/O and migration save flow - #627
Conversation
Improve robustness around network IO, disposal, migration and save behavior across sessions: - Core.Network.Session.cs: Add defensive checks and richer exception handling in the receive pipe loop (handle expected socket errors 995/10004, guard against disposed writer/advance races, suppress exceptions during disposal) and catch IO/Socket exceptions in SendRaw to log and disconnect cleanly. - Game/PacketHandlers/QuitHandler.cs: Add logging for MigrateOut RPC failures and annotate migration behavior to avoid prematurely dropping the migration packet (adjusted exception handling around migration send). - Game/Session/GameSession.cs: Change leave save to Async=false, refactor migration save flow by extracting SavePlayerState(), set preMigrationSaved earlier, and adjust error logging. Removed a local TrySaveComponent helper. - Login/Session/LoginSession.cs: Reorder disposal to call base.Dispose first to stop network operations, add Serilog debug logging for disposal errors. These changes are intended to make session teardown and migration more reliable and to avoid races/ignored errors when clients or sockets close during operations.
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdd defensive exception handling to core session receive/send paths, harden disposal flows in Login/Game sessions, and refactor GameSession migration save into a new SavePlayerState method for batched persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Socket
participant Session
participant PipeWriter as "Pipe Writer"
participant Consumer as "Recv Consumer"
Client->>Socket: send bytes
Socket->>Session: ReceiveAsync() returns buffer
Session->>PipeWriter: writer.Write(buffer)
PipeWriter->>Session: Advance()
Session->>PipeWriter: FlushAsync()
PipeWriter->>Consumer: data available
alt SocketException (995/10004) or disposed
Session->>Session: break loop / exit gracefully
end
sequenceDiagram
participant GameSession
participant SavePlayerState
participant Repo as "Persistence/Repo"
GameSession->>SavePlayerState: invoke during MigrationSave/Dispose
SavePlayerState->>Repo: batch-save components (UgcMarket,...,Dungeon)
Repo-->>SavePlayerState: success / error
alt error
SavePlayerState->>GameSession: log "SavePlayerState failed ..."
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Maple2.Server.Login/Session/LoginSession.cs`:
- Around line 167-175: The Dispose path currently calls
Server.OnDisconnected(this) and base.Dispose(disposing) inside one try, so if
Server.OnDisconnected throws the base.Dispose call is skipped; change
Dispose(bool disposing) to call Server.OnDisconnected(this) inside its own
try/catch (or catch and swallow/log) and ensure base.Dispose(disposing) runs in
a finally block (or its own try) so network shutdown always executes; also
replace Log.Logger.Debug(...) with the instance Logger.Debug(...) when logging
exceptions from Server.OnDisconnected to keep the inherited ForContext
enrichment.
🧹 Nitpick comments (3)
Maple2.Server.Game/Session/GameSession.cs (2)
850-854: SettingpreMigrationSaved = truebeforeSavePlayerState()— intentional but worth a note.If
SavePlayerState()throws, the flag prevents any retry duringDispose. This is likely intentional to avoid double-save races, but it means a transient DB failure during migration will result in unsaved player state with no recovery path. Consider whether a failed save should reset the flag or at least log at a higher severity.
856-883:SavePlayerState— partial commits possible when individual component saves fail.
TrySaveComponentswallows per-component exceptions, sodb.Commit()at line 872 will commit whatever succeeded. This means a player could end up with, e.g., saved items but lost quest progress. If atomicity is desired, the component exceptions should propagate to skip the commit. If partial saves are acceptable (existing behavior), this is fine as-is.Maple2.Server.Core/Network/Session.cs (1)
216-225: UseSocketErrorCodewithSocketErrorenum for cross-platform socket error handling.The raw error codes 995 and 10004 are Windows-specific (WinSock). On Linux/macOS,
SocketException.ErrorCodereturns platform-native values, making this comparison unreliable. .NET normalizes these codes viaSocketErrorCodeproperty, which maps to portableSocketErrorenum members:-} catch (SocketException sockEx) when (sockEx.ErrorCode == 995 || sockEx.ErrorCode == 10004) { +} catch (SocketException sockEx) when ( + sockEx.SocketErrorCode == SocketError.OperationAborted || + sockEx.SocketErrorCode == SocketError.Interrupted) {
2 participants
Improve robustness around network IO, disposal, migration and save behavior across sessions:
Core.Network.Session.cs: Add defensive checks and richer exception handling in the receive pipe loop (handle expected socket errors 995/10004, guard against disposed writer/advance races, suppress exceptions during disposal) and catch IO/Socket exceptions in SendRaw to log and disconnect cleanly.
Game/PacketHandlers/QuitHandler.cs: Add logging for MigrateOut RPC failures and annotate migration behavior to avoid prematurely dropping the migration packet (adjusted exception handling around migration send).
Game/Session/GameSession.cs: Change leave save to Async=false, refactor migration save flow by extracting SavePlayerState(), set preMigrationSaved earlier, and adjust error logging. Removed a local TrySaveComponent helper.
Login/Session/LoginSession.cs: Reorder disposal to call base.Dispose first to stop network operations, add Serilog debug logging for disposal errors.
These changes are intended to make session teardown and migration more reliable and to avoid races/ignored errors when clients or sockets close during operations.
Summary by CodeRabbit
Bug Fixes
Refactor
Logging