Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## Unreleased

### Bug Fixes 🐛

- (rails) Make Action Cable `handle_open`/`handle_close` overrides public so Rails 8.2's `ActionCable::Server::Socket` can invoke them ([rails/rails#50979](https://github.com/rails/rails/pull/50979)); the private overrides raised `NoMethodError` on every cable connection, killing all websocket traffic

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No need to update this as CHANGELOG is now updated automatically during the release process 🪄

## 6.6.2

### Bug Fixes 🐛
Expand Down
9 changes: 7 additions & 2 deletions sentry-rails/lib/sentry/rails/action_cable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,13 @@ def finish_transaction(transaction, status_code)
end

module Connection
private

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We need to do this conditionally, otherwise on older Rails version we'd be changing method visibility, and that would be an unwanted side-effect.


# These overrides must stay public: Rails 8.2 decoupled the Action
# Cable connection from the socket (rails/rails#50979), and
# ActionCable::Server::Socket now invokes connection.handle_open and
# connection.handle_close from outside the connection. With private
# overrides every cable connection raises NoMethodError before the
# welcome message is sent. On older Rails versions these methods are
# only called internally, so public visibility is harmless there.
def handle_open
ErrorHandler.capture(self, transaction_name: "#{self.class.name}#connect") do
super
Expand Down
14 changes: 14 additions & 0 deletions sentry-rails/spec/sentry/rails/action_cable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,20 @@ def disconnect
transport.events = []
end

describe "Connection method visibility" do
before do
make_basic_app
end

# Rails 8.2 (rails/rails#50979) invokes connection.handle_open and
# connection.handle_close from ActionCable::Server::Socket, outside the
# connection — private overrides break every cable connection there.
it "keeps handle_open and handle_close public" do
expect(Sentry::Rails::ActionCableExtensions::Connection.public_method_defined?(:handle_open)).to eq(true)
expect(Sentry::Rails::ActionCableExtensions::Connection.public_method_defined?(:handle_close)).to eq(true)
end
end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This tests implementation details. I assume that under Rails 8.2 some of our specs would fail, right? If they do, then this test is redundant, if they don't, then it would be good to reproduce the issue under 8.2 by exercising the actual behavior that Sentry adds.


describe "Connection" do
before do
make_basic_app
Expand Down