Skip to content

fix(websocket): guard ws.send/close in checkClients against EPIPE#1821

Merged
piorpua merged 1 commit intomainfrom
fix/sentry-ELECTRON-78
Mar 29, 2026
Merged

fix(websocket): guard ws.send/close in checkClients against EPIPE#1821
piorpua merged 1 commit intomainfrom
fix/sentry-ELECTRON-78

Conversation

@kaizhou-lab
Copy link
Copy Markdown
Collaborator

Summary

  • Guard ws.send() and ws.close() in WebSocketManager.checkClients() with readyState checks and try-catch to prevent unhandled EPIPE exceptions when the underlying socket pipe is broken
  • Fall back to ws.terminate() when graceful close fails
  • Apply the same defensive pattern to the heartbeat-timeout path and message-handler error response

Sentry Issue

  • ELECTRON-78Error: write EPIPE in WebSocketManager.checkClients (816 occurrences, ongoing)
  • Culprit: ws.send() called on a broken socket during token expiry handling without readyState guard or error handling

Root Cause

checkClients() sends an auth-expired message via ws.send() and then calls ws.close() without checking ws.readyState or wrapping in try-catch. When the client's socket pipe is already broken (common on low-resource Linux machines), this throws an unhandled EPIPE. Other methods like sendHeartbeat() and validateConnection() already had similar guards.

Test Plan

  • Unit test: EPIPE on ws.send() during auth-expired → falls back to ws.terminate()
  • Unit test: EPIPE on ws.close() during heartbeat timeout → falls back to ws.terminate()
  • Unit test: skip ws.send() when readyState !== OPEN for expired token
  • bun run test passes
  • bunx tsc --noEmit passes
  • bun run lint:fix clean
  • bun run format clean

Add readyState check and try-catch around ws.send() and ws.close() in
WebSocketManager.checkClients() so a broken pipe (EPIPE) no longer
propagates as an unhandled exception.  When the graceful close fails the
socket is terminated instead.  The same pattern is applied to the
heartbeat-timeout path and the message-handler error response.

Fixes ELECTRON-78
@kaizhou-lab kaizhou-lab marked this pull request as ready for review March 28, 2026 02:32
@piorpua piorpua added the bot:reviewing Review in progress (mutex) label Mar 29, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 29, 2026

Code Review:fix(websocket): guard ws.send/close in checkClients against EPIPE (#1821)

变更概述

本 PR 修复了 WebSocketManager.checkClients() 中因向已断开的 socket 调用 ws.send() / ws.close() 而抛出未处理 EPIPE 异常的问题(Sentry ELECTRON-78,816 次)。修改了 WebSocketManager.ts 的超时处理和 token 过期处理路径,新增 tests/unit/WebSocketManager.test.ts


方案评估

结论:✅ 方案合理

本 PR 精准定位根因,并复用了该类其他方法(sendHeartbeatvalidateConnection)已有的防御性模式:先检查 readyState,再 try-catch 包裹,失败后降级为 ws.terminate()。变更范围最小化,与项目架构保持一致,无引入不必要的抽象。


问题清单

🔵 LOW — 新增测试文件存在大量 no-explicit-any lint 警告

文件tests/unit/WebSocketManager.test.ts,第 22、34、39、56、60、68、82、91、104、107、109 行

问题说明:新测试文件产生了 16 个 typescript-eslint(no-explicit-any) 警告。虽然测试代码中 as any 用于 mock 访问私有成员是常见手法,但项目 strict mode 下这些均为 lint WARNING 级别问题。注意源文件中的 any 为存量问题,非本 PR 引入。


🔵 LOW — setupMessageHandler 新增的内层 catch 缺少对应测试

文件src/process/webserver/websocket/WebSocketManager.ts,第 139–149 行

问题说明:本 PR 在 setupMessageHandler 的错误响应路径中新增了内层 try-catch,但新增的测试文件中没有覆盖这一场景("当 JSON 解析失败且 ws.send 也抛 EPIPE 时应静默忽略")。三个测试均集中在 checkClients() 路径。


汇总

# 严重级别 文件 问题
1 🔵 LOW tests/unit/WebSocketManager.test.ts 大量 no-explicit-any lint 警告
2 🔵 LOW WebSocketManager.ts:139–149 setupMessageHandler 内层 catch 缺少测试

结论

批准合并 — 无阻塞性问题

修复精准有效,与现有防御性模式一致,测试覆盖了关键的 EPIPE 场景,两个 LOW 级问题均不阻塞合并。


本报告由本地 pr-review skill 生成,包含完整项目上下文,无截断限制。

@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 29, 2026

✅ 已自动 review,无阻塞性问题,正在触发自动合并。

@piorpua piorpua merged commit 6795344 into main Mar 29, 2026
17 checks passed
@piorpua piorpua deleted the fix/sentry-ELECTRON-78 branch March 29, 2026 08:13
@piorpua piorpua added bot:done Auto-merged by bot and removed bot:reviewing Review in progress (mutex) labels Mar 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:done Auto-merged by bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants