Skip to content

patch #2735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

patch #2735

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 58 additions & 0 deletions lib/concurrent_ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
require 'sentry-ruby'
require 'concurrent-ruby'

module Sentry
module ConcurrentRuby
SENTRY_THREAD_KEY = :sentry_hub
SENTRY_SPAN_KEY = :sentry_span

def self.install
patch_futures
patch_promises
end


def self.patch_futures
::Concurrent::Promises.singleton_class.class_eval do
alias_method :original_future, :future

def future(*args, &block)
current_hub = Sentry.get_current_hub
current_span = Sentry.get_current_scope.get_span
original_future(*args) do
#set the hub and span
Thread.current.thread_variable_set(Sentry::ConcurrentRuby::SENTRY_THREAD_KEY, current_hub)
Thread.current.thread_variable_set(Sentry::ConcurrentRuby::SENTRY_SPAN_KEY, current_span)
Sentry.with_child_span(op: "api.request", description: "Concurrent::Future") do |child_span|
Sentry.get_current_scope.set_span(child_span)
block.call
end
end
end
end
end


def self.patch_promises
::Concurrent::Promises::Future.class_eval do
alias_method :original_on_resolution, :on_resolution

def on_resolution(&block)
current_hub = Thread.current.thread_variable_get(Sentry::ConcurrentRuby::SENTRY_THREAD_KEY) || Sentry.get_current_hub
current_span = Sentry.get_current_scope.get_span
Comment on lines +41 to +42
Copy link

Choose a reason for hiding this comment

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

Potential bug: The on_resolution method retrieves current_hub from a thread variable but gets current_span from Sentry.get_current_scope, which may use a different hub, breaking trace context.

  • Description: In the on_resolution method, current_hub is retrieved from either a thread-local variable (SENTRY_THREAD_KEY) or Sentry.get_current_hub. However, current_span is always fetched via Sentry.get_current_scope.get_span, which internally calls Sentry.get_current_hub. If the thread variable contains a hub different from the current thread's hub (e.g., when a promise chain crosses threads), the span will be associated with the wrong hub. This leads to an incorrect span hierarchy and broken tracing information. The implementation also ignores the stored span in the SENTRY_SPAN_KEY thread variable.

  • Suggested fix: Retrieve the span from the same source as the hub. When current_hub is loaded from the thread variable, the corresponding span should also be loaded from its thread variable (SENTRY_SPAN_KEY) to ensure the hub and span contexts are consistent.
    severity: 0.6, confidence: 0.9

Did we get this right? 👍 / 👎 to inform future reviews.

original_on_resolution do |*args|
Thread.current.thread_variable_set(Sentry::ConcurrentRuby::SENTRY_THREAD_KEY, current_hub)
Thread.current.thread_variable_set(Sentry::ConcurrentRuby::SENTRY_SPAN_KEY, current_span)
Sentry.with_child_span(op: "api.request", description: "Concurrent::Promise") do |child_span|
Sentry.get_current_scope.set_span(child_span)
block.call(*args)
Copy link

Choose a reason for hiding this comment

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

Bug: Span Propagation Inconsistency

Span context propagation is incomplete. In patch_promises, current_span is retrieved from the current scope, ignoring the propagated thread variable. Both patch_futures and patch_promises also create a child span but don't restore the original span, which can lead to inconsistent scope state and incorrect span hierarchy.

Fix in Cursor Fix in Web

end
end
end
end
end
end
end

# Install after Sentry loaded
Sentry::ConcurrentRuby.install if defined?(Sentry)
Copy link

Choose a reason for hiding this comment

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

Potential bug: The integration accesses ::Concurrent::Promises without checking if the concurrent-ruby gem is loaded, which can cause a NameError on application startup.

  • Description: The Sentry::ConcurrentRuby.install method is called when lib/concurrent_ruby.rb is loaded. This method accesses constants like ::Concurrent::Promises. However, there is no check to ensure the concurrent-ruby gem has been loaded first. If a user includes this file without having concurrent-ruby in their Gemfile, the application will crash with a NameError during initialization. This behavior is inconsistent with other Sentry integrations, which perform a defined?() check on the dependency's constants before attempting to patch them.

  • Suggested fix: Add a check for the presence of the Concurrent::Promises constant before calling Sentry::ConcurrentRuby.install, similar to other integrations. For example: Sentry::ConcurrentRuby.install if defined?(Sentry) && defined?(::Concurrent::Promises).
    severity: 0.8, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.