Repository navigation
Conversation
Add `onPingTimeout` to `RpcClient.makeProtocolSocket` and `layerProtocolSocket`. It runs when the pinger drops the connection because no server frame arrived within `pingTimeout`, before `ConnectionHooks.onDisconnect` and before in-flight calls fail with `SocketReadError`. Defects are logged and ignored, like `onTransientError`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 21e20ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 32 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
This branch is waiting to be deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #8825, which closed #8774 and left this part open.
Problem
When an RPC socket client loses its connection,
ConnectionHooks.onDisconnectruns, but it cannot tell why. A ping timeout ("the server stopped answering") and a server-side close ("the server closed the connection") look the same to it. A client that shows connection state to a user needs to know which one happened: the first suggests a hung server or a dead network path, the second a restart or deliberate close.The reason is not observable from outside
makeProtocolSockettoday. The timeout fails the socket withSocketReadError({ cause: new Error("ping timeout") }), and that error reaches in-flight calls throughClientProtocolError. But a WebSocketerrorevent after open is also aSocketReadError, so the tag alone does not identify a timeout. Matching on the cause's message string is fragile.onDisconnectis anEffect<void>with no access to the error, and the error only reaches calls that are in flight or sent before the next reconnect. TheonTransientErrordocs say directly that a ping timeout "is not reported through this hook".T3 Code carries a local patch that adds an
onPingTimeouteffect toConnectionHooks. Our client sets a flag there and reads it inonDisconnectto choose between "${label}stopped responding." and "${label}disconnected.". It is the last thing we still patch ineffect, and this PR is the upstream version of it.Change
makeProtocolSocketandlayerProtocolSockettake a new optionalonPingTimeout: Effect<void>, next toonTransientError. It runs when the pinger's timeout drops a connection that had opened. The hook runs after the socket is torn down, so a concurrent socket error cannot interrupt it, and beforeConnectionHooks.onDisconnectand before in-flight calls fail with theSocketReadErrorabove. A ping timeout while the socket is still opening fails the attempt as before but does not run the hook. As withonTransientError, defects are logged and ignored so they cannot change how the connection fails. The JSDoc on both functions and ononTransientErrornow points to the new option.Why an option on
makeProtocolSocketrather than the alternatives:ConnectionHooks.onPingTimeout?(what our patch does). This also works and is additive. ButConnectionHooksis also read bymakeProtocolWorker, which has no pinger, so the member would be silently ignored there. The pinger's timing (pingInterval,pingTimeout) and the other socket-specific hook (onTransientError) are already options onmakeProtocolSocket, so this keeps the hook next to the setting that triggers it.onDisconnect. ChangingonDisconnectfromEffect<void>to a function breaks every existingConnectionHooksimplementation. An additive variant (a second, optionalonDisconnectWithReason) would need a public "disconnect reason" type to be designed first, which seems larger than this case warrants.If maintainers prefer the
ConnectionHooksshape, it is a small change to move.Testing
Two tests in
packages/effect/test/rpc/RpcClient.test.ts, next to the existing ping-timeout tests and usingTestClock. They recordonConnect,onPingTimeout,onDisconnect, and in-flight stream failures.SocketCloseError1006), and the hook does not run while the client stays disconnected past the ping timeout. The reconnected socket then goes silent. Its ping timeout runs the hook beforeonDisconnectand before the in-flight stream fails withSocketReadError.SocketReadErrorand runsonDisconnect, but not the hook.Frame liveness is already covered by the existing "keeps in-flight streams alive on non-pong frames" test, so there is no separate test for it.
pnpm vitest --run packages/effect/test/rpc/ packages/effect/test/cluster/Runners.test.ts: 5 files, 105 passed.RpcClient.tsreverted tomain, with the hook run on every socket failure, and with the hook run afteronDisconnect. The second fails when the hook is not limited to opened connections.pnpm lintandpnpm checkpass.🤖 Generated with Claude Code
Closes EFF-1905