Skip to content

fix: recover from panics in websocket handlers to prevent server crash#240

Merged
gmegidish merged 2 commits into
mainfrom
fix-websocket-panic-recovery
May 29, 2026
Merged

fix: recover from panics in websocket handlers to prevent server crash#240
gmegidish merged 2 commits into
mainfrom
fix-websocket-panic-recovery

Conversation

@gmegidish
Copy link
Copy Markdown
Member

a panic in any handler propagated up and killed the process; defer recover logs the method name and stack trace, then sends a clean error response

a panic in any handler propagated up and killed the process; defer recover
logs the method name and stack trace, then sends a clean error response
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ca224266-9ac5-4b26-aec3-aa6e51a4abab

📥 Commits

Reviewing files that changed from the base of the PR and between 8906ebb and a17de48.

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

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting


Walkthrough

This PR makes two independent changes. First, it reorganizes the go.mod file by consolidating module dependencies: most entries previously marked as // indirect in a secondary require block are moved into the primary block without the indirect comments, while a smaller secondary block retains only three indirect dependencies. Second, it enhances error handling in server/websocket.go by adding fmt and runtime/debug imports and wrapping the method invocation path with a deferred panic recovery handler that captures panics with stack traces and converts them to JSON-RPC server error responses.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

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.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding panic recovery to websocket handlers to prevent server crashes, which aligns with the primary modification in server/websocket.go.
Description check ✅ Passed The description is directly related to the changeset, explaining how panics in handlers are now caught and logged with stack traces before sending error responses, which matches the server/websocket.go modifications.
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-panic-recovery

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 `@go.mod`:
- Around line 6-53: The go.mod changes removed or altered // indirect markers
for many modules (the entries shown) though none are directly imported; restore
correct indirect/direct annotations by reverting the reorganization or running
"go mod tidy" to regenerate go.mod and go.sum, ensuring modules like
github.com/Masterminds/semver, github.com/sirupsen/logrus, golang.org/x/crypto,
etc. return to their proper indirect status; verify the go.mod matches the
output of "go mod tidy" and commit that result.
🪄 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: 10e89f54-8627-4773-8ab1-b1d18a852786

📥 Commits

Reviewing files that changed from the base of the PR and between a47cb8f and 8906ebb.

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

Comment thread go.mod
Comment on lines +6 to +53
al.essio.dev/pkg/shellescape v1.5.1
github.com/Masterminds/semver v1.5.0
github.com/cenkalti/backoff v2.2.1+incompatible
github.com/danieljoos/wincred v1.2.2
github.com/danielpaulus/go-ios v1.0.211
github.com/davecgh/go-spew v1.1.1
github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572
github.com/godbus/dbus/v5 v5.1.0
github.com/google/btree v1.1.2
github.com/google/pprof v0.0.0-20210407192527-94a9f03dee38
github.com/google/uuid v1.6.0
github.com/gorilla/websocket v1.5.3
github.com/grandcat/zeroconf v1.0.0
github.com/hashicorp/golang-lru/v2 v2.0.7
github.com/inconshreveable/mousetrap v1.1.0
github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0
github.com/miekg/dns v1.1.57
github.com/onsi/ginkgo/v2 v2.9.5
github.com/pierrec/lz4 v2.6.1+incompatible
github.com/pkg/errors v0.9.1
github.com/pmezard/go-difflib v1.0.0
github.com/quic-go/quic-go v0.40.1-0.20231203135336-87ef8ec48d55
github.com/sevlyar/go-daemon v0.1.6
github.com/sirupsen/logrus v1.9.3
github.com/songgao/water v0.0.0-20200317203138-2b4b6d7c09d8
github.com/spf13/cobra v1.9.1
github.com/spf13/pflag v1.0.6
github.com/stretchr/testify v1.10.0
github.com/tadglines/go-pkgs v0.0.0-20210623144937-b983b20f54f9
github.com/yapingcat/gomedia v0.0.0-20240906162731-17feea57090c
github.com/zalando/go-keyring v0.2.6
go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352
go.uber.org/mock v0.5.0
golang.org/x/crypto v0.45.0
golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842
golang.org/x/mod v0.29.0
golang.org/x/net v0.47.0
golang.org/x/sync v0.18.0
golang.org/x/sys v0.38.0
golang.org/x/text v0.31.0
golang.org/x/time v0.5.0
golang.org/x/tools v0.38.0
golang.zx2c4.com/wintun v0.0.0-20230126152724-0fa3db229ce2
gopkg.in/ini.v1 v1.67.0
gopkg.in/yaml.v3 v3.0.1
gvisor.dev/gvisor v0.0.0-20240405191320-0878b34101b5
howett.net/plist v1.0.1
software.sslmate.com/src/go-pkcs12 v0.2.0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check if packages marked as direct dependencies are actually imported

packages=(
  "al.essio.dev/pkg/shellescape"
  "github.com/Masterminds/semver"
  "github.com/cenkalti/backoff"
  "github.com/davecgh/go-spew"
  "github.com/go-task/slim-sprig"
  "github.com/godbus/dbus/v5"
  "github.com/google/btree"
  "github.com/google/pprof"
  "github.com/grandcat/zeroconf"
  "github.com/inconshreveable/mousetrap"
  "github.com/kardianos/osext"
  "github.com/miekg/dns"
  "github.com/onsi/ginkgo/v2"
  "github.com/pierrec/lz4"
  "github.com/pkg/errors"
  "github.com/pmezard/go-difflib"
  "github.com/songgao/water"
  "github.com/spf13/pflag"
  "github.com/tadglines/go-pkgs"
  "go.mozilla.org/pkcs7"
  "go.uber.org/mock"
  "golang.org/x/crypto"
  "golang.org/x/exp"
  "golang.org/x/mod"
  "golang.org/x/net"
  "golang.org/x/sync"
  "golang.org/x/sys"
  "golang.org/x/text"
  "golang.org/x/time"
  "golang.org/x/tools"
  "golang.zx2c4.com/wintun"
  "gopkg.in/yaml.v3"
  "gvisor.dev/gvisor"
  "software.sslmate.com/src/go-pkcs12"
)

echo "Checking which packages are directly imported in .go files..."
echo ""

for pkg in "${packages[@]}"; do
  # Escape dots and slashes for regex
  escaped=$(echo "$pkg" | sed 's/\./\\./g')
  
  # Search for import statements in .go files (excluding vendor and test files for cleaner output)
  matches=$(rg -t go --no-heading -n "\"$escaped" -g '!vendor/**' -g '!**/*_test.go' | head -5)
  
  if [ -n "$matches" ]; then
    echo "$pkg - FOUND (directly imported)"
    echo "$matches" | head -3
    echo ""
  else
    echo "$pkg - NOT FOUND (likely should be marked // indirect)"
  fi
done

echo ""
echo "To verify and fix, run: go mod tidy"

Repository: mobile-next/mobilecli

Length of output: 2736


🏁 Script executed:

#!/bin/bash
cd "$(git rev-parse --show-toplevel)" || exit 1

packages=(
  "al.essio.dev/pkg/shellescape"
  "github.com/Masterminds/semver"
  "github.com/cenkalti/backoff"
  "github.com/davecgh/go-spew"
  "github.com/go-task/slim-sprig"
  "github.com/godbus/dbus/v5"
  "github.com/google/btree"
  "github.com/google/pprof"
  "github.com/grandcat/zeroconf"
  "github.com/inconshreveable/mousetrap"
  "github.com/kardianos/osext"
  "github.com/miekg/dns"
  "github.com/onsi/ginkgo/v2"
  "github.com/pierrec/lz4"
  "github.com/pkg/errors"
  "github.com/pmezard/go-difflib"
  "github.com/songgao/water"
  "github.com/spf13/pflag"
  "github.com/tadglines/go-pkgs"
  "go.mozilla.org/pkcs7"
  "go.uber.org/mock"
  "golang.org/x/crypto"
  "golang.org/x/exp"
  "golang.org/x/mod"
  "golang.org/x/net"
  "golang.org/x/sync"
  "golang.org/x/sys"
  "golang.org/x/text"
  "golang.org/x/time"
  "golang.org/x/tools"
  "golang.zx2c4.com/wintun"
  "gopkg.in/yaml.v3"
  "gvisor.dev/gvisor"
  "software.sslmate.com/src/go-pkcs12"
)

echo "Checking direct imports in .go files (including *_test.go), excluding vendor..."
echo ""

for pkg in "${packages[@]}"; do
  escaped=$(printf "%s" "$pkg" | sed 's/\./\\./g')
  matches=$(rg -t go --no-heading -n "\"$escaped" -g '!vendor/**' | head -3)
  if [ -n "$matches" ]; then
    echo "$pkg - FOUND"
    echo "$matches"
    echo ""
  else
    echo "$pkg - NOT FOUND"
  fi
done

Repository: mobile-next/mobilecli

Length of output: 1425


go.mod: revert/remove // indirect changes for unimported dependencies (go.mod lines 6-53)

Repo-wide scan found none of the listed modules being imported anywhere in non-vendored .go files (including *_test.go). Removing // indirect from these entries is therefore likely incorrect; run go mod tidy (or revert the dependency reorganization) to restore correct indirect/direct requirements.

🤖 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 `@go.mod` around lines 6 - 53, The go.mod changes removed or altered //
indirect markers for many modules (the entries shown) though none are directly
imported; restore correct indirect/direct annotations by reverting the
reorganization or running "go mod tidy" to regenerate go.mod and go.sum,
ensuring modules like github.com/Masterminds/semver, github.com/sirupsen/logrus,
golang.org/x/crypto, etc. return to their proper indirect status; verify the
go.mod matches the output of "go mod tidy" and commit that result.

Combines PR #240 (defer recover in handlers) with main's goroutine +
semaphore pattern. Panic recovery now runs inside the handler goroutine,
after the semaphore is acquired. Also updates go-ios to v1.0.211.
@gmegidish gmegidish merged commit 82befd1 into main May 29, 2026
9 of 10 checks passed
@gmegidish gmegidish deleted the fix-websocket-panic-recovery branch May 29, 2026 19:44
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