Skip to content

fix: websocket timeouts after around 50-60 seconds#239

Merged
gmegidish merged 2 commits into
mainfrom
fix-websocket-timeouts
May 26, 2026
Merged

fix: websocket timeouts after around 50-60 seconds#239
gmegidish merged 2 commits into
mainfrom
fix-websocket-timeouts

Conversation

@gmegidish
Copy link
Copy Markdown
Member

No description provided.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

Walkthrough

The PR adds a buffered semaphore field to wsConnection and a wsMaxConcurrentHandlers constant, initializes the semaphore in NewWebSocketHandler, and changes handleWSMethodCall to attempt a non-blocking acquire of the semaphore and run handlers in a goroutine (releasing the semaphore when done). If acquisition fails, a JSON-RPC server-error is returned immediately. sendJSON now calls SetWriteDeadline(wsWriteWait) before WriteJSON and returns any deadline-setting error.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess relevance to the changeset. Add a pull request description explaining the timeout issue, root cause, and how the semaphore-based handler offloading with write deadlines resolves it.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the fix being implemented - addressing websocket timeouts occurring around 50-60 seconds.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-websocket-timeouts

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/websocket.go`:
- Around line 210-222: The code spawns an unbounded goroutine per WebSocket
request; add a bounded concurrency semaphore to limit concurrent handlers (e.g.,
add a semaphore field like handlerSem on the wsConn or a package-level limiter)
and acquire a token before starting the goroutine (if acquisition fails respond
with ErrCodeServerError/ResourceExhausted via wsConn.sendError), then run the
handler inside the goroutine and defer release of the token after handler
finishes; update the block that calls handler(req.Params) to acquire the token,
spawn the goroutine only when acquired, and ensure wsConn.sendResponse and
wsConn.sendError still execute inside the goroutine and after the token is
released.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a2c3cceb-6eb9-4d52-8d10-9a45abf9b669

📥 Commits

Reviewing files that changed from the base of the PR and between 0ebdd70 and 3c4c967.

📒 Files selected for processing (1)
  • server/websocket.go

Comment thread server/websocket.go
adds a buffered-channel semaphore on wsConnection to bound goroutine growth;
requests beyond the limit are rejected immediately with a server error response
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
server/websocket.go (1)

223-233: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Recover panics inside the handler goroutine (handleWSMethodCall)

handler(req.Params) runs in a spawned goroutine, but the defer only releases handlerSem; there’s no recover(), so any panic in the handler can crash the whole server.

🛡️ Minimal hardening patch
 	go func() {
-		defer func() { <-wsConn.handlerSem }()
+		defer func() {
+			<-wsConn.handlerSem
+			if r := recover(); r != nil {
+				log.Printf("panic executing method %s: %v", req.Method, r)
+				_ = wsConn.sendError(req.ID, ErrCodeServerError, "Server error", "internal handler panic")
+			}
+		}()
 		result, err := handler(req.Params)
 		if err != nil {
 			log.Printf("Error executing method %s: %v", req.Method, err)
 			wsConn.sendError(req.ID, ErrCodeServerError, "Server error", err.Error())
 			return
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/websocket.go` around lines 223 - 233, The spawned goroutine calling
handler(req.Params) must recover from panics to avoid crashing the server: wrap
the body with a defer that calls recover(), logs the panic (including req.Method
and stack trace), sends a server error back via wsConn.sendError(req.ID,
ErrCodeServerError, "Server error", <panic info>), and still releases the
handlerSem; modify the anonymous goroutine in handleWSMethodCall so the existing
defer that reads from handlerSem is preserved and a new defer handles recover(),
logging and sending the error when a panic occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@server/websocket.go`:
- Around line 223-233: The spawned goroutine calling handler(req.Params) must
recover from panics to avoid crashing the server: wrap the body with a defer
that calls recover(), logs the panic (including req.Method and stack trace),
sends a server error back via wsConn.sendError(req.ID, ErrCodeServerError,
"Server error", <panic info>), and still releases the handlerSem; modify the
anonymous goroutine in handleWSMethodCall so the existing defer that reads from
handlerSem is preserved and a new defer handles recover(), logging and sending
the error when a panic occurs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 69bca0fe-dfc4-4531-8c77-449fe50c4b2b

📥 Commits

Reviewing files that changed from the base of the PR and between 3c4c967 and 85e5625.

📒 Files selected for processing (1)
  • server/websocket.go

@gmegidish gmegidish merged commit c89676e into main May 26, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant