Fix WebSocket TOCTOU race conditions and concurrent write panics (#402) - #461
Fix WebSocket TOCTOU race conditions and concurrent write panics (#402)#461dhruvv16-hash wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe patch synchronizes initial and control WebSocket writes. It also changes room admission and disconnect cleanup locking so room membership checks and registry updates occur under coordinated locks. ChangesWebSocket concurrency
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change serializes WebSocket writes and room membership updates, preventing concurrent write failures and incorrect room admission. No concrete current-head merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant WebsocketHandler
participant roomsMutex
participant roomMutex
participant websocketUpgrader
participant roomClients
WebsocketHandler->>websocketUpgrader: Upgrade connection
WebsocketHandler->>roomsMutex: Lock room registry
WebsocketHandler->>roomMutex: Lock selected room
WebsocketHandler->>roomClients: Validate assignment and register client
WebsocketHandler->>roomMutex: Unlock selected room
WebsocketHandler->>roomsMutex: Unlock room registry
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
🟡 Changes recommended
TeamWebsocketHandler currently attempts to write an HTTP JSON error response after the WebSocket upgrade, which is unreliable/incorrect once the connection is upgraded.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses concurrency hazards in the backend WebSocket layer by tightening room lifecycle locking to prevent TOCTOU “ghost rooms” and by routing WebSocket writes through per-connection mutex-protected helpers to avoid concurrent write panics.
Changes:
- Reordered locking in room join/leave paths to ensure room deletion checks are performed safely under consistent mutex ordering.
- Moved WebSocket upgrades out of shared map critical sections and replaced unsafe raw writes (
WriteJSON/WriteMessage) with safe wrappers using per-client write mutexes. - Added a safe raw-message writer for gamification WebSockets and applied it to ping/pong handling.
File summaries
| File | Description |
|---|---|
| backend/websocket/websocket.go | Adjusts room join/disconnect locking to avoid TOCTOU deletion races; switches a remaining direct write to SafeWriteJSON. |
| backend/websocket/team_websocket.go | Adjusts team room creation/join/disconnect locking and moves upgrade earlier in the handler. |
| backend/websocket/gamification.go | Adds SafeWriteMessage to protect raw WebSocket writes with the client write mutex. |
| backend/websocket/gamification_handler.go | Uses SafeWriteMessage for pong responses to avoid concurrent write panics. |
| backend/websocket/debate_spectator.go | Routes initial snapshot/presence writes through the spectator client writer (mutex-protected). |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if userTeamIDHex != team1IDHex && userTeamIDHex != team2IDHex { | ||
| log.Printf("[TeamWebsocketHandler] ❌ ERROR: UserTeamID %s doesn't match Team1ID %s or Team2ID %s", userTeamIDHex, team1IDHex, team2IDHex) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Team assignment error"}) | ||
| room.Mutex.Unlock() | ||
| conn.Close() | ||
| return | ||
| } |
| "isMuted": shouldBeMuted, | ||
| "currentTurn": currentTurn, | ||
| "phase": message.Phase, | ||
| } | ||
| if err := clientConn.WriteJSON(response); err != nil { | ||
| if err := client.SafeWriteJSON(response); err != nil { |
Link your account with GitcordThanks for opening this PR, @dhruvv16-hash! To receive Discord notifications and contributor tracking for this organization:
Once linked, Gitcord can notify you about reviews, merges, and more. — Posted by Gitcord |
Hey everyone! 👋
The Problem:
This PR resolves several critical concurrency bugs in the WebSocket handling:
websocket.goandteam_websocket.go, a Time-Of-Check to Time-Of-Use race condition existed where a client joining a room could lock it and add themselves right after the last client left and triggered a room deletion. This left new clients stranded in an orphaned "ghost" room structure that was removed from the main map.WriteJSONandWriteMessagefunctions are not safe for concurrent use. There were several locations inwebsocket.go,debate_spectator.go, andgamification_handler.gowhere rawconn.WriteJSONorconn.WriteMessagewere invoked simultaneously alongside the broadcast goroutines invokingSafeWriteJSON, causing panics.The Fix:
roomsMutexbefore checkingroom.Mutexwhen evaluating room deletion in the disconnect loops.upgrader.Upgradeoutside of the critical map lookup section for safety.conn.WriteJSONandconn.WriteMessageoccurrences with safe wrappers (client.SafeWriteJSONorclient.WriteJSON) that utilize the client's dedicatedwriteMumutex lock.Fixes #402
Summary by CodeRabbit