Skip to content

Commit 427612b

Browse files
solnicclaude
andauthored
fix(tests): proper dummy transport clean up for hub cloning (#2957)
* test(test_helper): add failing regression specs for clone_hub event leak (#2951) Captures the documented bug where setup_sentry_test is ineffective in request specs because clone_hub_to_current_thread clones @main_hub rather than the thread-local hub setup_sentry_test mutated, and DummyTransport has no #clear, so events from a prior unrelated request leak into a later test via the main hub's base-layer DummyTransport. - Reproduction spec (intentional TDD red): after a setup/teardown cycle, an intermediate clone_hub_to_current_thread + capture, then a second setup_sentry_test, sentry_events must be empty both immediately and after a further clone_hub_to_current_thread. Currently the second assertion fails because the stale event is still visible. - Guard spec (passes today, must keep passing): events captured through a hub cloned by the Rack middleware after setup_sentry_test remain observable via sentry_events, so the eventual fix cannot regress by silently dropping request-captured events. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(test_helper): add DummyTransport#clear to empty captured events clear_sentry_events called #clear only if the transport responded to it; DummyTransport never did, so clearing the testing transport was a silent no-op. Add #clear that empties the captured events and envelopes in place (keeping array references valid). Refs #2951 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test_helper): anchor setup/teardown to the main hub (#2951) setup_sentry_test mutated the thread-local hub, but Sentry.clone_hub_to_current_thread (used by Sentry::Rack::CaptureExceptions) always clones the *main* hub. After an intermediate clone the thread-local hub is a clone, so a later setup_sentry_test reconfigured the clone while the main hub kept a stale DummyTransport on its base layer. Combined with clear being a no-op for that transport, an unrelated request's event leaked into the next test's sentry_events (intermittent under RSpec random ordering). - setup_sentry_test now binds the base and test clients on the main hub and realigns the current thread's hub to it, so direct sentry_events reads and request-time clones share one DummyTransport. - teardown_sentry_test pops the testing layer off the main hub (where setup pushed it) instead of the current thread's hub. - clear_sentry_events now clears every transport reachable from the current hub and the main hub (including its base layer) via the new sentry_test_transports helper, so no stale DummyTransport survives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(test_helper): add Rack two-request leak regression spec (#2951) Drives two consecutive requests through the real Sentry::Rack::CaptureExceptions middleware (which calls clone_hub_to_current_thread) wrapped in setup_sentry_test/ teardown_sentry_test, asserting the first request's event does not leak into the second and that each request's event stays observable via sentry_events. Fails against pre-fix code (second request sees 2 events); passes with the #2951 fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(test_helper): use public Hub#clients instead of @stack ivar (#2951) Address code review feedback on the #2951 fix: - Add a documented public Sentry::Hub#clients accessor returning all clients across the scope stack, and use it from TestHelper#sentry_test_transports instead of reaching into Hub's private @stack ivar. This keeps the test helper resilient to future Hub internals refactors. Covered by new hub_spec.rb examples. - Replace the final inline teardown_sentry_test in the #2951 regression and Rack consecutive-requests describe blocks with an unconditional 'after { teardown_sentry_test }' hook, matching the sibling describe style so cleanup runs even if an expectation fails mid-example. The intentional mid-scenario teardowns remain inline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 9be586e commit 427612b

5 files changed

Lines changed: 144 additions & 8 deletions

File tree

sentry-ruby/lib/sentry/hub.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ def current_client
5454
current_layer&.client
5555
end
5656

57+
# All clients bound across the hub's scope stack, base layer first.
58+
# @return [Array<Client>]
59+
def clients
60+
@stack.map(&:client).compact
61+
end
62+
5763
def configuration
5864
current_client.configuration
5965
end

sentry-ruby/lib/sentry/test_helper.rb

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,25 @@ def setup_sentry_test(&block)
3737
# - auto_session_tracking
3838
block&.call(dummy_config)
3939

40+
# Install the testing clients on the *main* hub rather than the current
41+
# thread's hub. `Sentry.clone_hub_to_current_thread` (used by
42+
# Sentry::Rack::CaptureExceptions) always clones the main hub, so if we
43+
# only mutated the thread-local hub a request-time clone would observe a
44+
# stale transport.
45+
main_hub = Sentry.get_main_hub
46+
4047
# the base layer's client should already use the dummy config so nothing will be sent by accident
4148
base_client = Sentry::Client.new(dummy_config)
42-
Sentry.get_current_hub.bind_client(base_client)
49+
main_hub.bind_client(base_client)
4350
# create a new layer so mutations made to the testing scope or configuration could be simply popped later
44-
Sentry.get_current_hub.push_scope
51+
main_hub.push_scope
4552
test_client = Sentry::Client.new(dummy_config.dup)
46-
Sentry.get_current_hub.bind_client(test_client)
53+
main_hub.bind_client(test_client)
54+
55+
# Realign the current thread's hub with the main hub so direct
56+
# `sentry_events` reads and any hub the Rack middleware clones from the
57+
# main hub all observe the same DummyTransport.
58+
Thread.current.thread_variable_set(Sentry::THREAD_LOCAL, main_hub)
4759
end
4860

4961
# Clears all stored events and envelopes.
@@ -54,19 +66,29 @@ def teardown_sentry_test
5466

5567
clear_sentry_events
5668

57-
# pop testing layer created by `setup_sentry_test`
58-
# but keep the base layer to avoid nil-pointer errors
69+
# pop the testing layer created by `setup_sentry_test` off the *main*
70+
# hub (that is where `setup_sentry_test` pushed it), keeping the base
71+
# layer to avoid nil-pointer errors. Popping the current thread's hub
72+
# would leave the test layer dangling on the main hub, which the next
73+
# request-time clone would inherit.
5974
# TODO: find a way to notify users if they somehow popped the test layer before calling this method
60-
if Sentry.get_current_hub.instance_variable_get(:@stack).size > 1
61-
Sentry.get_current_hub.pop_scope
75+
main_hub = Sentry.get_main_hub
76+
if main_hub.instance_variable_get(:@stack).size > 1
77+
main_hub.pop_scope
6278
end
6379
Sentry::Scope.global_event_processors.clear
6480
end
6581

6682
def clear_sentry_events
6783
return unless Sentry.initialized?
6884

69-
sentry_transport.clear if sentry_transport.respond_to?(:clear)
85+
# Clear every transport reachable from the current thread's hub and the
86+
# main hub (including its base layer). A request-time clone shares the
87+
# main hub's base-layer transport, so clearing only the current
88+
# transport would let stale events survive into the next test.
89+
sentry_test_transports.each do |transport|
90+
transport.clear if transport.respond_to?(:clear)
91+
end
7092

7193
if Sentry.configuration.enable_logs && sentry_logger.respond_to?(:clear)
7294
sentry_logger.clear
@@ -83,6 +105,17 @@ def sentry_transport
83105
Sentry.get_current_client.transport
84106
end
85107

108+
# Every transport reachable from the current thread's hub and the main
109+
# hub, across all stack layers. Used by `clear_sentry_events` so a stale
110+
# DummyTransport (e.g. the main hub's base layer that a request-time clone
111+
# shares) cannot carry leftover events into the next test.
112+
# @return [Array<Transport>]
113+
def sentry_test_transports
114+
[Sentry.get_current_hub, Sentry.get_main_hub].compact.uniq.flat_map do |hub|
115+
hub.clients.map(&:transport)
116+
end.compact.uniq
117+
end
118+
86119
# Returns the captured event objects.
87120
# @return [Array<Event>]
88121
def sentry_events

sentry-ruby/lib/sentry/transport/dummy_transport.rb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,12 @@ def send_event(event)
1818
def send_envelope(envelope)
1919
@envelopes << envelope
2020
end
21+
22+
# Empties the captured events and envelopes so `TestHelper.clear_sentry_events`
23+
# also clears the dummy transport instance
24+
def clear
25+
@events.clear
26+
@envelopes.clear
27+
end
2128
end
2229
end

sentry-ruby/spec/sentry/hub_spec.rb

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -537,6 +537,20 @@
537537
end
538538
end
539539

540+
describe "#clients" do
541+
it "returns the only client for a single-layer hub" do
542+
expect(subject.clients).to eq([client])
543+
end
544+
545+
it "returns every client across the scope stack, base layer first" do
546+
new_client = Sentry::Client.new(configuration)
547+
subject.push_scope
548+
subject.bind_client(new_client)
549+
550+
expect(subject.clients).to eq([client, new_client])
551+
end
552+
end
553+
540554
describe "#pop_scope" do
541555
it "pops the current scope" do
542556
prev_scope = subject.current_scope

sentry-ruby/spec/sentry/test_helper_spec.rb

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,82 @@
9292
end
9393
end
9494

95+
describe "event leakage across clone_hub_to_current_thread (regression for #2951)" do
96+
after { teardown_sentry_test }
97+
98+
it "keeps sentry_events empty after setup_sentry_test even when an earlier request captured events through a cloned hub" do
99+
setup_sentry_test
100+
Sentry.capture_message("event from a previous test")
101+
teardown_sentry_test
102+
103+
Sentry.clone_hub_to_current_thread
104+
Sentry.capture_message("event from an unrelated request")
105+
106+
setup_sentry_test
107+
108+
expect(sentry_events).to be_empty
109+
110+
Sentry.clone_hub_to_current_thread
111+
112+
expect(sentry_events).to be_empty
113+
end
114+
end
115+
116+
describe "request-captured events remain observable after clone_hub_to_current_thread" do
117+
after { teardown_sentry_test }
118+
119+
it "still exposes events captured through a hub the Rack middleware cloned after setup_sentry_test" do
120+
setup_sentry_test
121+
122+
Sentry.clone_hub_to_current_thread
123+
Sentry.capture_message("event from the request")
124+
125+
expect(sentry_events.map(&:message)).to include("event from the request")
126+
end
127+
end
128+
129+
describe "Sentry::Rack::CaptureExceptions across consecutive requests (regression for #2951)", when: :rack_available? do
130+
after { teardown_sentry_test }
131+
132+
# Drives a single request through the real Rack middleware. The middleware
133+
# calls Sentry.clone_hub_to_current_thread before handing off to the app,
134+
# exactly like a Rails request spec would.
135+
def perform_request(exception_message)
136+
exception = RuntimeError.new(exception_message)
137+
app = lambda do |env|
138+
env["rack.exception"] = exception
139+
[200, {}, ["ok"]]
140+
end
141+
stack = Sentry::Rack::CaptureExceptions.new(app)
142+
stack.call(Rack::MockRequest.env_for("/#{exception_message}"))
143+
end
144+
145+
def captured_exception_messages
146+
sentry_events.map { |event| event.to_h.dig(:exception, :values, 0, :value) }
147+
end
148+
149+
it "isolates each request's events and keeps them observable via sentry_events" do
150+
# First request, wrapped in the test helper just like a request spec.
151+
setup_sentry_test
152+
perform_request("first-request")
153+
messages = captured_exception_messages
154+
expect(messages.size).to eq(1)
155+
expect(messages.first).to include("first-request")
156+
teardown_sentry_test
157+
158+
# Second request: a fresh setup must not see the first request's event,
159+
# even though the Rack middleware clones the main hub on every request.
160+
setup_sentry_test
161+
expect(sentry_events).to be_empty
162+
163+
perform_request("second-request")
164+
messages = captured_exception_messages
165+
expect(messages.size).to eq(1)
166+
expect(messages.first).to include("second-request")
167+
expect(messages).not_to include(a_string_including("first-request"))
168+
end
169+
end
170+
95171
describe "#teardown_sentry_test" do
96172
before do
97173
setup_sentry_test

0 commit comments

Comments
 (0)