Skip to content

Fix websocket panics on session disconnect#2269

Open
jackmaninov wants to merge 1 commit intocortezaproject:2024.9.xfrom
jackmaninov:fix-websocket-panics
Open

Fix websocket panics on session disconnect#2269
jackmaninov wants to merge 1 commit intocortezaproject:2024.9.xfrom
jackmaninov:fix-websocket-panics

Conversation

@jackmaninov
Copy link
Copy Markdown

Summary

Fixes multiple race conditions and error handling issues in websocket session management that caused panics during unauthenticated visitor access and login redirects.

Problem

When unauthenticated users connect via WebSocket (e.g., during redirect from base URL to login page), several issues can occur:

  1. Nil pointer dereference: disconnect() sets conn = nil while read()/write() operations are pending, causing panic when they try to use the connection
  2. Read loop spinning: errHandler() returned nil for close errors, causing the read loop to continue instead of exiting. After ~1000 iterations, gorilla/websocket triggers a safety panic
  3. Send on closed channel: Write() could attempt to send to s.send channel after disconnect() closed it
  4. Noisy error logs: Expected close conditions (1006 abnormal closure, "close sent") were logged as errors

Changes

  • Add nil checks in read() and write() after acquiring lock to prevent nil pointer dereference
  • Return net.ErrClosed from errHandler() for close errors to exit read/write loops cleanly
  • Add atomic.Bool closed flag to session, set before closing channels in disconnect()
  • Check closed flag in Write() before attempting channel send
  • Handle additional expected close codes (1005, 1006) and "close sent" error in errHandler()

Testing

  • Existing websocket tests pass
  • Manually verified with unauthenticated visitor access and login redirect flow

Fixes #2223

Fixes multiple race conditions and error handling issues in websocket
session management that caused panics during unauthenticated visitor
access and login redirects.

Changes:
- Add nil checks in read() and write() after acquiring lock to prevent
  nil pointer dereference when disconnect() races with read/write
- Return net.ErrClosed from errHandler() for close errors to prevent
  read loop spinning (~1000 iterations before panic)
- Check context in Write() before sending to detect disconnecting session
- Add atomic.Bool closed flag to session, checked before channel send
  to prevent "send on closed channel" panics
- Handle additional expected close codes (1005, 1006) and "close sent"
  error to suppress noisy error logs during normal disconnection

Fixes cortezaproject#2223

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@jackmaninov
Copy link
Copy Markdown
Author

Not sure if you have an AI generated code policy, but this was done supervised and tested on my production environment.

@Fajfa
Copy link
Copy Markdown
Member

Fajfa commented Jan 5, 2026

If the code is good and resolves the issue, then it will be accepted. Thanks for contributing.

@Fajfa Fajfa requested a review from tjerman January 5, 2026 08:20
@Fajfa Fajfa added this to the 2024.9.8 milestone Jan 8, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

Stale pull request message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Websocket write panic on redirect to login screen

3 participants