Repository navigation
fix(realtime): preserve terminal disconnect reasons and codes - #352
chaser-kong wants to merge 1 commit into
Conversation
|
|
tkkhq
left a comment
There was a problem hiding this comment.
Per the repository rollout guidance, validate the merged SDK main revision through the coordinated Hosting staging scenario. The PR reports live acceptance as pending; this approval does not verify that acceptance.
AI review: 2 P0-P2 finding(s)The PR keeps WebSocket close codes on 🟠 P1
🟡 P2
⚪ P3
Not checked: The AI review read the diff, the code, and the PR's text; it ran no tests or builds and read nothing outside this repository.
Push a fix, or have a collaborator with write access (including the PR author) add the Human review: needed
Decide the points above: approve to accept them, or request changes. Suggested reviewer: |
|
|
||
| class ClosedError < StandardError; end | ||
| # A closed realtime transport, retaining its numeric close code when available. | ||
| class ClosedError < ServerError |
There was a problem hiding this comment.
🟡 P2: Making ClosedError a subclass of ServerError changes which rescue clauses catch transport closes
ClosedError and ServerError used to be separate StandardError subclasses, so callers could tell a server rejection apart from a dropped connection. A common pattern is rescue Volcano::Realtime::ServerError => e; report(e.code) followed by rescue Volcano::Realtime::ClosedError; retry_later. After this change the first clause catches every close, and the retry branch never runs.
The class comment for ServerError still says "Error returned by the realtime server for a command". Yet it now also covers errors the client raises on its own: the manual-disconnect 'realtime connection closed', and 'realtime channel is not subscribed' from Channel#send and #track.
Inside the SDK, reject_failed_subscription (channel_lifecycle.rb:114) treats error.is_a?(ServerError) as "server rejected, connection still healthy" and skips protocol.close. That is safe today only because every protocol ClosedError currently implies the protocol is already closed.
Suggested fix: keep ClosedError < StandardError with its own code reader. Add a ClosedError branch in Redaction.copy_exception so the code survives redaction. error_context already uses respond_to?(:code), so the code still reaches on_error.
Found by claude-opus-5-5
| elsif push.key?('leave') | ||
| dispatch_presence(push, 'leave') | ||
| elsif push.key?('disconnect') | ||
| close_with(ClosedError.from_disconnect(push.fetch('disconnect')), notify_error: true) |
There was a problem hiding this comment.
⚪ P3: Disconnect-push reasons skip credential redaction, unlike WebSocket close reasons
closed_error passes the server's WebSocket close reason through Redaction.message(..., secrets: @secrets). ClosedError.from_disconnect stores the server-supplied reason as it is. That error becomes @closed_error and goes to on_error and on_disconnect contexts. Those contexts don't redact it: immutable_exception uses secrets: [], and disconnect_context reads error.message directly. Only replies to pending requests are redacted later, inside request.
So the same reason string is redacted when it arrives in a close frame but not when it arrives in a disconnect push. That contradicts the new docs line "Credentials are redacted from error messages". Suggested fix: redact the reason before building the error, for example by passing Redaction.message(reason, secrets: @secrets) into from_disconnect from dispatch_push.
Found by claude-opus-5-5
| @@ -1,5 +1,7 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'protocol/websocket/error' | |||
There was a problem hiding this comment.
⚪ P3: Top-level require of an undeclared transitive dependency at require 'volcano' time
lib/volcano.rb:69 loads realtime/protocol eagerly, so every require 'volcano' now loads protocol/websocket/error. Elsewhere the realtime stack defers its async-related requires (load_async and method-local require 'async/...'). Also, protocol-websocket isn't declared in volcano-sdk.gemspec; it is only available because the pinned async-websocket depends on it. Two options: declare the dependency, or avoid the eager require with defined?(::Protocol::WebSocket::ClosedError) && error.is_a?(::Protocol::WebSocket::ClosedError). If such an error was raised, the constant is already loaded.
Found by claude-opus-5-5
| elsif push.key?('leave') | ||
| dispatch_presence(push, 'leave') | ||
| elsif push.key?('disconnect') | ||
| close_with(ClosedError.from_disconnect(push.fetch('disconnect')), notify_error: true) |
There was a problem hiding this comment.
🟠 P1: Redact disconnect reasons before notifying lifecycle callbacks
A disconnect push whose reason contains the access token or encoded API key passes that credential directly to close_with. ConnectionCallbacks#protocol_failed delivers it through on_error and, for established connections, on_disconnect; its exception snapshot uses secrets: []. Consequently, the documented callback logging can expose credentials even though the pending request's exception is redacted later. Redact the error with the protocol's secrets before closing, preserving its code, and cover callback messages in a regression test.
Found by gpt-6.1-sol
Change
Realtime startup lost the server's numeric code when a WebSocket closed, and ignored protocol disconnect pushes until a pending request timed out.
Preserve close-frame codes in
Volcano::Realtime::ClosedErrorand process disconnect pushes immediately. Keep reason/code metadata through credential redaction, publish typed RBS signatures, and document the exception. Existing startup socket disposal remains in place; Ruby did not share JavaScript's retained-client leak. Native command errors already preserve their metadata and remain unchanged.Verification
Tested Ruby revision:
0d9b9f9.3507: permission deniedand left the facade disconnected.go test ./scripts/cipassed; the installed gem discovered all 59 canonical scenarios in the isolated Docker runner. Discovery and loopback checks are not live staging acceptance.Cross-language impact
Classification: public facade, shared behavior, native behavior, and public examples. No wire/OpenAPI snapshot change.
SDK-REALTIME-005— terminal startup rejection retains the server's reason/code, reports disconnected, and releases its transport.a80904b). Python: fix(realtime): surface terminal startup rejection reasons volcano-sdk-python#399 (55e38ff). Hosting: https://github.com/Kong/volcano-hosting/pull/1925 (0e2fc6439).fix/realtime-connection-rejectionbranch. There are no SDK-owned active/staged copies.docs/realtime.md.Realtime::ClosedErrorwith a typed optional numericcode; Python exportsRealtimeConnectionError, and JavaScript exposes a class/type guard for explicitconnect()rejections.