Skip to content

fix(realtime): preserve terminal disconnect reasons and codes - #352

Open
chaser-kong wants to merge 1 commit into
mainfrom
fix/realtime-connection-rejection
Open

chaser-kong wants to merge 1 commit into
mainfrom
fix/realtime-connection-rejection

Conversation

@chaser-kong

@chaser-kong chaser-kong commented Oct 15, 2026 •

Copy link
Copy Markdown

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::ClosedError and 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.

  • Both startup regression tests failed before the fix.
  • Dependency audit, deterministic OpenAPI check, RuboCop (398 files), RBS validation, and both Steep projects passed.
  • Full RSpec suite: 2,046 examples passed; 100% line and branch coverage. Cases cover real upstream WebSocket close errors, protocol disconnect pushes, missing metadata, and malformed pushes.
  • All five injected-defect checks detected their target defects.
  • Isolated gem installation and documented synthetic-HTTP quickstart passed.
  • Real Async/WebSocket loopback rejected promptly with 3507: permission denied and left the facade disconnected.
  • Hosting contract tooling: 90 tests and go test ./scripts/ci passed; 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.

  • Requirement: SDK-REALTIME-005 — terminal startup rejection retains the server's reason/code, reports disconnected, and releases its transport.
  • JavaScript: fix(realtime): surface terminal connection rejection reasons volcano-sdk-js#329 (a80904b). Python: fix(realtime): surface terminal startup rejection reasons volcano-sdk-python#399 (55e38ff). Hosting: https://github.com/Kong/volcano-hosting/pull/1925 (0e2fc6439).
  • Canonical Gherkin and all three bindings are maintained together on Hosting's matching fix/realtime-connection-rejection branch. There are no SDK-owned active/staged copies.
  • Equivalent public examples are updated in each SDK's docs/realtime.md.
  • Ruby uses Realtime::ClosedError with a typed optional numeric code; Python exports RealtimeConnectionError, and JavaScript exposes a class/type guard for explicit connect() rejections.
  • Merge the SDK implementations before validating the coordinated Hosting scenario against SDK main revisions. Live staging acceptance remains pending Staging Validation; no cloud fixtures were provisioned locally.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@tkkhq tkkhq 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.

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.

@chaser-kong
chaser-kong marked this pull request as ready for review October 15, 2026 22:01
@chaser-kong
chaser-kong requested a review from a team as a code owner October 15, 2026 22:01
@github-actions

Copy link
Copy Markdown

AI review: 2 P0-P2 finding(s)

Reviewed 0d9b9f9. Run log

The PR keeps WebSocket close codes on Realtime::ClosedError and makes Centrifuge disconnect push frames close the protocol right away. It also adds RBS signatures, docs, and startup regression specs. The runtime logic looks right. The main risk is that ClosedError now subclasses the public Realtime::ServerError, which changes how callers' rescue clauses match, and the description doesn't mention it.

🟠 P1

  • lib/volcano/realtime/protocol_dispatch.rb:33 Redact disconnect reasons before notifying lifecycle callbacks

🟡 P2

  • lib/volcano/realtime/protocol.rb:26 Making ClosedError a subclass of ServerError changes which rescue clauses catch transport closes

⚪ P3

  • lib/volcano/realtime/protocol_dispatch.rb:33 Disconnect-push reasons skip credential redaction, unlike WebSocket close reasons
  • lib/volcano/realtime/connection_callbacks.rb:99 on_disconnect still reports code nil even though the close error now carries the code
  • lib/volcano/realtime/protocol_io.rb:3 Top-level require of an undeclared transitive dependency at require 'volcano' time

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.

  • Whether the Volcano realtime server actually sends disconnect pushes and close frames shaped the way this code expects (integer code, string reason); only live staging shows this.
  • Whether the coordinated Hosting scenario (Kong/volcano-hosting#1925) and the JS/Python exception shapes match this Ruby behavior.
  • Whether a real async-websocket reader loop ends promptly when the SDK closes the socket while handling a disconnect push in the middle of a frame.

Push a fix, or have a collaborator with write access (including the PR author) add the ai-review-override label to accept 0d9b9f9 as is. An override also needs a human review.

Human review: needed

  • Should public Realtime::ClosedError subclass Realtime::ServerError (protocol.rb)? rescue ServerError would start catching closes, and the PR description doesn't mention it.
  • Is making ClosedError inherit from ServerError in lib/volcano/realtime/protocol.rb acceptable for consumers whose existing rescue ServerError handlers will now catch local and transport closures?

Decide the points above: approve to accept them, or request changes.

Suggested reviewer: swkeever (main committer to realtime protocol, dispatch, IO, and RBS files). Any collaborator with write access other than the author can approve.


class ClosedError < StandardError; end
# A closed realtime transport, retaining its numeric close code when available.
class ClosedError < ServerError

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ 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'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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

This branch has not been deployed

No deployments
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.

3 participants