Skip to content

fix(client): reject calls when websocket is not open instead of sending into a closed socket#1581

Merged
dinwwwh merged 2 commits into
middleapi:mainfrom
strblr:fix/ws-link-closed-socket-send
Jun 12, 2026
Merged

fix(client): reject calls when websocket is not open instead of sending into a closed socket#1581
dinwwwh merged 2 commits into
middleapi:mainfrom
strblr:fix/ws-link-closed-socket-send

Conversation

@strblr

@strblr strblr commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Problem

LinkWebsocketClient calls websocket.send() without checking the socket state. With a plain WebSocket this doesn't matter, but with reconnecting sockets like partysocket (which the docs recommend) it breaks in two ways:

  • Frames sent while disconnected are buffered and replayed onto the next connection. By then the client peer has already rejected those calls and forgotten their ids (the close handler calls peer.close()), so the server ends up with ghost requests, e.g. duplicate event-iterator subscriptions streaming everything twice (or N times), and mutations that get re-executed.
  • With maxEnqueuedMessages: 0 on partysocket's side: the frame is silently dropped, but the peer still awaits a response forever. The call never settles, so caller retry logic never runs.

Easy to reproduce: subscribe to an event iterator, kill the server for ~20s while the client retries, bring it back. Each retry leaves one buffered REQUEST frame, and on reconnect they all become live server-side subscriptions.

Fix

Each send now dispatches on readyState:

  • OPEN => send, as before.
  • CONNECTING => wait for the next open/close event, then re-check. This covers the initial connect (replacing the constructor-level untilOpen) and also reconnect windows on reconnecting sockets: a call made while an attempt is in flight is sent once the connection opens, or rejected if the attempt fails, instead of erroring out during a routinely-occurring ~50ms window.
  • CLOSING/CLOSED => throw. peer.request() already handles a throwing send by rejecting the call, so callers get an immediate error instead of a hang or a ghost frame.

Native WebSocket behavior is unchanged: CONNECTING only happens before the first open, and after a close all pending calls are rejected anyway. The guard only turns a send into a dead socket from a silent no-op into a proper rejection.

One behavioral note: frames can no longer be buffered across a reconnect by the socket wrapper. I think that's correct: request ids are connection-scoped, so replaying frames onto a new connection is never safe, and callers already have to handle rejection since close rejects everything in flight. (The wait-during-CONNECTING path is safe for the same reason in reverse: if a call is rejected while its frame is waiting, ClientPeer's id-guard drops the frame instead of sending it.)

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working javascript Pull requests that update javascript code labels Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

WebSocket send now enforces open-state at send time by adding WEBSOCKET_OPEN, expanding the websocket type, waiting out CONNECTING, and throwing if readyState is not OPEN. Tests add removeEventListener mocks, align open-state in a send test, and add a non-open sockets suite for edge cases.

Changes

WebSocket readiness enforcement

Layer / File(s) Summary
Send-time readiness guard
packages/client/src/adapters/websocket/link-client.ts
Adds WEBSOCKET_OPEN constant, requires removeEventListener on the websocket option, waits while readyState === CONNECTING (listening for open or close), and throws if websocket.readyState is not WEBSOCKET_OPEN before sending.
Readiness guard and reconnect tests
packages/client/src/adapters/websocket/rpc-link.test.ts
Adds removeEventListener to websocket mocks, sets websocket.readyState = 1 in the open test, and adds a non-open sockets suite covering starting non-open (reject, no send), mid-flight close (subsequent rejects, send counts preserved), waiting through reconnect then sending, and failing reconnect (reject without send).

Possibly related PRs

  • middleapi/orpc#615: Related prior work on WebSocket open state validation; both address guarding sends against non-OPEN sockets.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I peek through the wire with curious whisker twitch,
Listening for "open" before I hitch my pitch,
If the socket is closed, I bop and refrain,
Wait for the hello, then hop on the train,
Safe sends, happy hops — that's the rabbit's wish.

🚥 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
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(client): reject calls when websocket is not open instead of sending into a closed socket' accurately and clearly describes the main change: adding a readyState check to reject calls rather than sending into a closed socket.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds a check to ensure the WebSocket connection is open before sending messages, throwing an error if it is not, and includes tests to verify this behavior. The review feedback correctly points out a potential hang if the WebSocket fails to connect initially, as the 'untilOpen' promise only listens for the 'open' event. To resolve this, it is recommended to also listen for the 'close' event to prevent requests from hanging indefinitely.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread packages/client/src/adapters/websocket/link-client.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes — Prevents LinkWebsocketClient from sending frames into a closed or connecting socket by checking readyState before send(), rejecting the call when the socket isn't open. This avoids ghost requests and hung promises with reconnecting wrappers like partysocket.

  • Guard websocket.send() with readyState check — after await untilOpen, link-client.ts throws if readyState !== WEBSOCKET_OPEN, so frames never reach a dead socket.
  • Two new tests + one existing-test fix — "rejects instead of sending when socket is not open" (fresh CLOSED socket), "rejects instead of sending when socket closes between requests" (OPEN → CLOSED transition), and the existing "waits until open" test now sets readyState = 1 before onOpen() to satisfy the new guard.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jun 11, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes — Adds close event listener during the CONNECTING wait so requests that arrive while the socket is connecting don't hang forever when the initial connection fails, and provides cleanup via removeEventListener.

  • Wait for close as well as open during CONNECTING — the send callback now listens for both events (with cleanup) so a failed initial connect resolves the wait and falls through to the "not open" rejection instead of hanging.
  • Four new tests in non-open sockets describe block — covers closed-from-start, closed-between-requests, reconnect-window with successful reopen, and failed-reconnect-while-connecting scenarios.
  • removeEventListener wired into mocks — existing websocket mocks and the new createWebSocket helper gain removeEventListener to match the updated Pick<> type.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@dinwwwh

dinwwwh commented Jun 12, 2026

Copy link
Copy Markdown
Member

Frames sent while disconnected are buffered and replayed onto the next connection. By then the client peer has already rejected those calls and forgotten their ids (the close handler calls peer.close()), so the server ends up with ghost requests, e.g. duplicate event-iterator subscriptions streaming everything twice (or N times), and mutations that get re-executed.

When clientPeer.close() is triggered by the close event, all in-flight requests are immediately rejected and cleaned up. As a result, the client does not continue send those requests after reconnection. Only requests created after the close event are queued and sent once the connection is re-established, so the scenario described as "ghost requests" should not occur.

With maxEnqueuedMessages: 0 on partysocket's side: the frame is silently dropped, but the peer still awaits a response forever. The call never settles, so caller retry logic never runs.

I agree this is a real issue. Ideally, PartySocket would provide an alternative behavior that throws an error when maxEnqueuedMessages is exceeded instead of silently dropping messages. That would allow pending requests to fail immediately and enable retry logic to run. For now, though, I think the most practical approach is to document this limitation clearly and add a warning in the docs.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 12, 2026
@dinwwwh dinwwwh changed the title fix(client/websocket): reject calls when socket is not open instead of sending into a closed socket fix(client): reject calls when socket is not open instead of sending into a closed socket Jun 12, 2026
@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@pkg-pr-new

pkg-pr-new Bot commented Jun 12, 2026

Copy link
Copy Markdown
More templates

@orpc/ai-sdk

npm i https://pkg.pr.new/@orpc/ai-sdk@1581

@orpc/arktype

npm i https://pkg.pr.new/@orpc/arktype@1581

@orpc/client

npm i https://pkg.pr.new/@orpc/client@1581

@orpc/contract

npm i https://pkg.pr.new/@orpc/contract@1581

@orpc/experimental-durable-iterator

npm i https://pkg.pr.new/@orpc/experimental-durable-iterator@1581

@orpc/hey-api

npm i https://pkg.pr.new/@orpc/hey-api@1581

@orpc/interop

npm i https://pkg.pr.new/@orpc/interop@1581

@orpc/json-schema

npm i https://pkg.pr.new/@orpc/json-schema@1581

@orpc/nest

npm i https://pkg.pr.new/@orpc/nest@1581

@orpc/openapi

npm i https://pkg.pr.new/@orpc/openapi@1581

@orpc/openapi-client

npm i https://pkg.pr.new/@orpc/openapi-client@1581

@orpc/otel

npm i https://pkg.pr.new/@orpc/otel@1581

@orpc/experimental-pino

npm i https://pkg.pr.new/@orpc/experimental-pino@1581

@orpc/experimental-publisher

npm i https://pkg.pr.new/@orpc/experimental-publisher@1581

@orpc/experimental-publisher-durable-object

npm i https://pkg.pr.new/@orpc/experimental-publisher-durable-object@1581

@orpc/experimental-ratelimit

npm i https://pkg.pr.new/@orpc/experimental-ratelimit@1581

@orpc/react

npm i https://pkg.pr.new/@orpc/react@1581

@orpc/react-query

npm i https://pkg.pr.new/@orpc/react-query@1581

@orpc/experimental-react-swr

npm i https://pkg.pr.new/@orpc/experimental-react-swr@1581

@orpc/server

npm i https://pkg.pr.new/@orpc/server@1581

@orpc/shared

npm i https://pkg.pr.new/@orpc/shared@1581

@orpc/solid-query

npm i https://pkg.pr.new/@orpc/solid-query@1581

@orpc/standard-server

npm i https://pkg.pr.new/@orpc/standard-server@1581

@orpc/standard-server-aws-lambda

npm i https://pkg.pr.new/@orpc/standard-server-aws-lambda@1581

@orpc/standard-server-fastify

npm i https://pkg.pr.new/@orpc/standard-server-fastify@1581

@orpc/standard-server-fetch

npm i https://pkg.pr.new/@orpc/standard-server-fetch@1581

@orpc/standard-server-node

npm i https://pkg.pr.new/@orpc/standard-server-node@1581

@orpc/standard-server-peer

npm i https://pkg.pr.new/@orpc/standard-server-peer@1581

@orpc/svelte-query

npm i https://pkg.pr.new/@orpc/svelte-query@1581

@orpc/tanstack-query

npm i https://pkg.pr.new/@orpc/tanstack-query@1581

@orpc/trpc

npm i https://pkg.pr.new/@orpc/trpc@1581

@orpc/valibot

npm i https://pkg.pr.new/@orpc/valibot@1581

@orpc/vue-colada

npm i https://pkg.pr.new/@orpc/vue-colada@1581

@orpc/vue-query

npm i https://pkg.pr.new/@orpc/vue-query@1581

@orpc/zod

npm i https://pkg.pr.new/@orpc/zod@1581

commit: 6222c1e

@dinwwwh dinwwwh changed the title fix(client): reject calls when socket is not open instead of sending into a closed socket fix(client): reject calls when websocket is not open instead of sending into a closed socket Jun 12, 2026
@dinwwwh

dinwwwh commented Jun 12, 2026

Copy link
Copy Markdown
Member

Easy to reproduce: subscribe to an event iterator, kill the server for ~20s while the client retries, bring it back. Each retry leaves one buffered REQUEST frame, and on reconnect they all become live server-side subscriptions.

Please provide more detail and repro for this

@strblr

strblr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@dinwwwh It happens consistently on prod apps that I sadly can't show here. A repro is trivial, you probably don't even need it to understand immediately what's going on. :

  • On a client that uses oRPC + partysocket, subscribe to an infinite event iterator with automatic retry (for example through useQuery retry option, whatever)

  • On first ws connect, untilOpen gets resolved forever (in LinkWebsocketClient)

  • On disconnect and reconnect, untilOpen is still resolved

  • So the await untilOpen gate doesn't check anything past the first disconnect

  • So any frame that the client wants to send to the server after that actually reaches partysocket. From there, two things can happen, both bad:

  • If you allow partysocket to buffer while disconnected: on reconnect it fires as many frames to the server as there were retries. The oRPC client has long forgotten about them but the server will never know and will send the events duplicated to the client. oRPC will not process them, but the packets will transit.

  • If you don't allow partysocket to buffer, then the retry will hang forever.

The root cause is that oRPC's ws link client was written with the native (non-reconnecting) WebSocket in mind. In that case, it is correct to have untilOpen resolve only once, because if it's ever closed it can't reopen. For partysocket, this is wrong and the correct semantic is to have an untilOpen equivalent on every frame while the socket is not open. That's the fix of this PR.

Fundamentally, the issue is in oRPC's link client, it can't be properly fixed on partysocket side nor in the retry logic.

@dinwwwh

dinwwwh commented Jun 12, 2026

Copy link
Copy Markdown
Member

If you allow partysocket to buffer while disconnected: on reconnect it fires as many frames to the server as there were retries. The oRPC client has long forgotten about them but the server will never know and will send the events duplicated to the client. oRPC will not process them, but the packets will transit.

Why "fires as many frames to the server as there were retries"?, retry should only happen one time and want until queued-frame send to server and server response.

@dinwwwh

dinwwwh commented Jun 12, 2026

Copy link
Copy Markdown
Member

There are still some issues when relying on something like PartySocket for retries, but from what I can tell this PR only addresses the maxEnqueuedMessages: 0 problem. (thinking about new approach in v2)

Messages no longer enter the queue because send now waits for the connection to open first. However, if reconnection fails, send should throw and the close event should fire again (assuming I'm understanding this correctly). In that case, all currently pending requests would still be rejected rather than hanging indefinitely.

@strblr

strblr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@dinwwwh Because the call doesn't stay pending until the buffered frame is delivered, oRPC rejects it first, on every failed reconnect attempt.

LinkWebsocketClient does:

websocket.addEventListener('close', () => {
  this.peer.close()  // rejects ALL pending calls, forgets their ids
})

and partysocket emits a close event on every failed reconnect attempt, not just once.

The retry can't "wait for the queued frame" because from the client peer's perspective the call already failed. And it can't not reject on close either, cause for in-flight calls on a genuinely dead connection that rejection is correct.

So the fix has to be at the send boundary: never hand a frame to a socket that isn't OPEN (wait while CONNECTING, reject otherwise). Then a frame's lifetime cannot outlive the call id it belongs to, and the buffer-replay issue disappears regardless of partysocket settings.

@strblr

strblr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

There are still some issues when relying on something like PartySocket for retries

With the fix in this PR, you mechanically NEVER rely on partysocket for retries. It either waits in oRPC or fails in oRPC.

@dinwwwh

dinwwwh commented Jun 12, 2026

Copy link
Copy Markdown
Member

and partysocket emits a close event on every failed reconnect attempt, not just once.

Do you mean that when a reconnect attempt fails, PartySocket keeps the queued messages instead of clearing them?

If so, the condition for reproducing this issue would be allowing PartySocket to retry multiple times. Each failed reconnect attempt emits another close event while the queued messages remain buffered.

@strblr

strblr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Yes, failed attempts emit close but never clear the queue. Everything in it is replayed onto the next successful connection.

That's not a PartySocket bug though. Buffering across reconnects is its intended feature, and it works fine for protocols where messages are connection-independent. oRPC's protocol isn't: request ids are only valid while ClientPeer remembers them, and peer.close() forgets them on every close event. So a buffered frame outlives the call it belongs to. PartySocket can't know that, and no setting fixes it: buffering replays dead ids, and maxEnqueuedMessages: 0 drops the frame while the call hangs forever.

Only oRPC knows whether a frame's id is still alive, so the check has to live in oRPC: never hand a frame to a non-OPEN socket, wait while CONNECTING, reject otherwise. Then frames can't sit anywhere oRPC doesn't control, and any reconnecting wrapper works out of the box.

@dinwwwh dinwwwh merged commit 47dbd93 into middleapi:main Jun 12, 2026
5 checks passed
@dinwwwh

dinwwwh commented Jun 12, 2026

Copy link
Copy Markdown
Member

Thanks, this fixes some problems, but I think we still need a more powerful retry mechanism in v2 - something that actually understands the protocol.

@strblr

strblr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

It's probably adjacent to the current fix, no matter who does the retrying, oRPC must not give the socket a message for a request it has already dropped. Without that guarantee at the send level, a v2 retry would hit the same problems. Thanks for the merge!

@strblr strblr deleted the fix/ws-link-closed-socket-send branch June 12, 2026 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working javascript Pull requests that update javascript code lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants