From 6a933d93a609cdda6b1cd2c52983119c79185e5a Mon Sep 17 00:00:00 2001 From: Arjun Rajappa Date: Tue, 7 Jul 2026 20:00:29 +0530 Subject: [PATCH 1/2] test: add regression tests for OTel context-stack corruption Add test/instrumentation/otel_context_leak_test.rb covering five instrumentation paths that previously left a non-SpanContext object as the of a non-recording span, causing NoMethodError on the next start_span call on the same thread. Each test is named after the component and observable behaviour rather than an opaque scenario letter, so failures are self-describing in CI output. Tests are written against the expected fixed behaviour and are intended to be merged before the corresponding source fixes land, serving as executable acceptance criteria. Covered paths: - Rack: context token not detached when agent is down (rack.rb) - rest-client: explicit with_parent on poisoned stack (rest-client.rb) - net-http: implicit Context.current fallback, both guard paths (net-http.rb) - gRPC: corrupted parent_context passed directly as with_parent (grpc.rb) - ActionCable: context stored from Rack-poisoned thread (action_cable.rb) Signed-off-by: Arjun Rajappa --- instana.gemspec | 2 + test/trace/otel_context_leak_test.rb | 241 +++++++++++++++++++++++++++ 2 files changed, 243 insertions(+) create mode 100644 test/trace/otel_context_leak_test.rb diff --git a/instana.gemspec b/instana.gemspec index 0277f4f6..e9025d77 100644 --- a/instana.gemspec +++ b/instana.gemspec @@ -40,6 +40,8 @@ Gem::Specification.new do |spec| spec.add_development_dependency "fakefs" spec.add_development_dependency "irb" spec.add_development_dependency "benchmark" + spec.add_development_dependency "rack" + spec.add_runtime_dependency('base64', '>= 0.1') spec.add_runtime_dependency('logger') diff --git a/test/trace/otel_context_leak_test.rb b/test/trace/otel_context_leak_test.rb new file mode 100644 index 00000000..1fee3616 --- /dev/null +++ b/test/trace/otel_context_leak_test.rb @@ -0,0 +1,241 @@ +# (c) Copyright IBM Corp. 2026 + +require 'test_helper' +require 'rack/mock_request' + +# Regression tests for the OTel context-stack corruption bugs where certain +# instrumentation paths left a non-SpanContext object as the `context` of a +# non-recording span, causing `NoMethodError` on the next `start_span` call. +# +# Each scenario maps directly to a root-cause analysis documented in the +# companion file `test_app_scenarios.rb`. +class OtelContextLeakTest < Minitest::Test + # ----------------------------------------------------------------------- + # Setup / Teardown + # ----------------------------------------------------------------------- + + def setup + @rack_app = Rack::Builder.new do + use Instana::Rack + run ->(_env) { [200, { 'content-type' => 'text/plain' }, ['ok']] } + end + @mock_request = Rack::MockRequest.new(@rack_app) + end + + def teardown + # Reset the OTel context stack to ROOT unconditionally. attach+detach is + # a no-op here because detach restores the *previous* value (the leaked + # frame), not ROOT. Context.clear resets the thread-local directly. + OpenTelemetry::Context.clear + + ::Instana.tracer.clear! + Instana.agent.define_singleton_method(:ready?) { true } + ::Instana.config[:allow_exit_as_root] = false + clear_all! + end + + # ----------------------------------------------------------------------- + # Shared helpers + # ----------------------------------------------------------------------- + + # Builds and attaches the corrupted OTel context frame that the unpatched + # Rack middleware would leave behind — a non_recording_span whose `.context` + # returns an OpenTelemetry::Context instead of a SpanContext. + # + # Yields the attached token; always detaches on exit regardless of outcome. + def with_corrupted_frame(trace_id: 'abc123', span_id: 'def456') + span_ctx = Instana::SpanContext.new(trace_id: trace_id, span_id: span_id) + nrs = OpenTelemetry::Trace.non_recording_span(span_ctx) + corrupted_ctx = Instana::Trace.context_with_span(nrs) # OTel Context, not SpanContext + corrupted_span = OpenTelemetry::Trace.non_recording_span(corrupted_ctx) # .context => OTel Context + leaked_frame = OpenTelemetry::Trace.context_with_span(corrupted_span) + token = OpenTelemetry::Context.attach(leaked_frame) + yield token + ensure + OpenTelemetry::Context.detach(token) + end + + # Assert that the span returned by start_span is a live, recording Instana span. + def assert_recording_span(span, msg = nil) + assert_instance_of Instana::Span, span, msg || 'Expected an Instana::Span' + assert span.recording?, "#{msg || 'Span'} must be recording" + end + + # ----------------------------------------------------------------------- + # RACK — context-stack detach (agent down + upstream headers) + # + # When the agent is down and a request arrives with upstream Instana trace + # headers, Rack builds a parent_context (an OTel Context), attaches it, and + # — before the fix — never detached it because the `detach` call was inside + # the `tracing?` guard. The poisoned frame survived to the next request, + # which crashed with NoMethodError in tracer_provider / samplers. + # + # Fix: rack.rb ensure block unconditionally calls `Context.detach(@trace_token)`. + # ----------------------------------------------------------------------- + + def test_rack_detaches_context_token_when_agent_is_down + Instana.agent.define_singleton_method(:ready?) { false } + + res = @mock_request.get( + '/', + 'HTTP_X_INSTANA_T' => 'abc123', + 'HTTP_X_INSTANA_S' => 'def456', + 'HTTP_X_INSTANA_L' => '1' + ) + + assert_equal 200, res.status + assert_equal OpenTelemetry::Context::ROOT, OpenTelemetry::Context.current, + 'Rack must detach its context token even when the agent is down' + end + + def test_rack_recovers_cleanly_after_upstream_headers_arrive_while_agent_is_down + Instana.agent.define_singleton_method(:ready?) { false } + @mock_request.get( + '/', + 'HTTP_X_INSTANA_T' => 'abc123', + 'HTTP_X_INSTANA_S' => 'def456', + 'HTTP_X_INSTANA_L' => '1' + ) + + Instana.agent.define_singleton_method(:ready?) { true } + + res = @mock_request.get('/') + assert_equal 200, res.status + # The context stack must be clean again after a successful traced request. + assert_equal OpenTelemetry::Context::ROOT, OpenTelemetry::Context.current, + 'Rack must restore context to ROOT after a normal traced request' + end + + # ----------------------------------------------------------------------- + # REST-CLIENT — explicit with_parent on poisoned stack + # + # rest-client.rb forwards `OpenTelemetry::Context.current` as `with_parent` + # to `start_span`. When the thread's context stack holds a corrupted frame, + # this bypasses the `||=` fallback guard and routes the bad context directly + # into `internal_start_span`, hitting the crash in samplers.rb. + # + # Fix: samplers.rb guards `.tracestate` with `respond_to?`. + # ----------------------------------------------------------------------- + + def test_rest_client_start_span_tolerates_poisoned_otel_context_on_stack + with_corrupted_frame(trace_id: 'abc123', span_id: 'def456') do + span = ::Instana.tracer.start_span(:'rest-client', with_parent: OpenTelemetry::Context.current) + assert_recording_span span, 'rest-client span' + span.finish + end + end + + # ----------------------------------------------------------------------- + # NET-HTTP — implicit Context.current fallback on poisoned stack + # + # net-http.rb calls `start_span(:'net-http')` without `with_parent`, so + # tracer.rb falls back to `OpenTelemetry::Context.current`. + # + # When allow_exit_as_root=false (default): `tracing?` returns false when + # `current_span` is nil, so `skip_instrumentation?` short-circuits and + # `start_span` is never reached — no crash, span not recorded (expected). + # + # When allow_exit_as_root=true: `tracing?` returns true, the guard is + # bypassed, and `start_span` is called with the poisoned context. + # + # Fix: same as REST-CLIENT — samplers.rb guards `.tracestate`. + # ----------------------------------------------------------------------- + + def test_net_http_skips_instrumentation_when_not_tracing_and_exit_as_root_is_off + with_corrupted_frame(trace_id: 'aaa111', span_id: 'bbb222') do + # Reproduce the skip_instrumentation? logic from net-http.rb line 70-73. + # `started?` is an instance method of Net::HTTP and is not testable here + # without a live connection, so we omit it — it is not relevant to the + # poisoned-context path being exercised. + dnt_spans = %i[dynamodb sqs sns s3] + skip = !Instana.tracer.tracing? || + !Instana.config[:nethttp][:enabled] || + (!::Instana.tracer.current_span.nil? && + dnt_spans.include?(::Instana.tracer.current_span.name)) + + assert skip, 'net-http must skip instrumentation when tracing? is false' + refute Instana.tracer.tracing?, + 'tracing? must be false when current_span is nil and allow_exit_as_root is off' + end + end + + def test_net_http_start_span_tolerates_poisoned_otel_context_when_exit_as_root_is_on + ::Instana.config[:allow_exit_as_root] = true + + with_corrupted_frame(trace_id: 'aaa111', span_id: 'bbb222') do + assert Instana.tracer.tracing?, 'tracing? must be true when allow_exit_as_root is on' + + span = ::Instana.tracer.start_span(:'net-http') + assert_recording_span span, 'net-http span' + span.finish + end + end + + # ----------------------------------------------------------------------- + # GRPC — corrupted parent_context passed directly as with_parent + # + # grpc.rb builds a parent_context via `non_recording_span` + + # `Trace.context_with_span`, producing an OTel Context (not a SpanContext), + # then passes it directly as `with_parent: parent_context`. With the agent + # up this reaches `internal_start_span` → samplers.rb → crash. + # + # Fix: same as REST-CLIENT — samplers.rb guards `.tracestate`. + # ----------------------------------------------------------------------- + + def test_grpc_start_span_tolerates_context_wrapping_otel_context_as_parent + # Replicate exactly what GRPCServerInstrumentation#handle_* does when + # upstream Instana headers are present. + incoming_context = Instana::SpanContext.new( + trace_id: ::Instana::Util.header_to_id('deadbeef'), + span_id: ::Instana::Util.header_to_id('cafebabe') + ) + nrs = OpenTelemetry::Trace.non_recording_span(incoming_context) + parent_context = Instana::Trace.context_with_span(nrs) # OTel Context — the corrupted form + + span = ::Instana.tracer.start_span(:'rpc-server', with_parent: parent_context) + assert_recording_span span, 'rpc-server span' + span.finish + end + + # ----------------------------------------------------------------------- + # ACTION CABLE — context stored from Rack-poisoned thread + # + # When the WebSocket upgrade request runs on a Rack-poisoned thread (agent + # was down, upstream headers present), `OpenTelemetry::Context.current` + # returns the corrupted frame. ActionCable stores that as `@instana_trace_context`. + # A later transmit/dispatch_action wraps it in `non_recording_span`, then + # calls `start_span` via `in_span`, which fires the crash. + # + # Fix: rack.rb detach fix ensures the poison is gone before ActionCable ever + # reads `Context.current`; samplers.rb fix provides a second line of + # defence if any other path produces the same corrupted shape. + # ----------------------------------------------------------------------- + + def test_action_cable_start_span_tolerates_context_stored_from_rack_poisoned_thread + # Step 1: Rack-poison the thread (agent down + upstream headers). + Instana.agent.define_singleton_method(:ready?) { false } + @mock_request.get( + '/', + 'HTTP_X_INSTANA_T' => 'dead01', + 'HTTP_X_INSTANA_S' => 'beef02', + 'HTTP_X_INSTANA_L' => '1' + ) + + # Step 2: Agent recovers. ActionCable#process reads Context.current to + # store the trace context for the WebSocket session. + Instana.agent.define_singleton_method(:ready?) { true } + stored_ctx = if ::Instana.tracer.tracing? + ::Instana.tracer.current_span.context + else + OpenTelemetry::Context.current + end + + # Step 3: A later transmit/dispatch_action wraps stored_ctx and starts a span. + nrs_cable = OpenTelemetry::Trace.non_recording_span(stored_ctx) + Instana::Trace.with_span(nrs_cable) do + span = ::Instana.tracer.start_span(:'rpc-server') + assert_recording_span span, 'rpc-server span in ActionCable dispatch' + span.finish + end + end +end From 73c4aacf8bc56f799308901ab49a9a17b7f08fbe Mon Sep 17 00:00:00 2001 From: Arjun Rajappa Date: Tue, 7 Jul 2026 20:37:47 +0530 Subject: [PATCH 2/2] test: fix linting failures Signed-off-by: Arjun Rajappa --- instana.gemspec | 1 - test/trace/otel_context_leak_test.rb | 10 +++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/instana.gemspec b/instana.gemspec index e9025d77..b637e66a 100644 --- a/instana.gemspec +++ b/instana.gemspec @@ -42,7 +42,6 @@ Gem::Specification.new do |spec| spec.add_development_dependency "benchmark" spec.add_development_dependency "rack" - spec.add_runtime_dependency('base64', '>= 0.1') spec.add_runtime_dependency('logger') spec.add_runtime_dependency('concurrent-ruby', '>= 1.1') diff --git a/test/trace/otel_context_leak_test.rb b/test/trace/otel_context_leak_test.rb index 1fee3616..ff391320 100644 --- a/test/trace/otel_context_leak_test.rb +++ b/test/trace/otel_context_leak_test.rb @@ -15,7 +15,7 @@ class OtelContextLeakTest < Minitest::Test # ----------------------------------------------------------------------- def setup - @rack_app = Rack::Builder.new do + @rack_app = Rack::Builder.new do use Instana::Rack run ->(_env) { [200, { 'content-type' => 'text/plain' }, ['ok']] } end @@ -46,7 +46,7 @@ def teardown def with_corrupted_frame(trace_id: 'abc123', span_id: 'def456') span_ctx = Instana::SpanContext.new(trace_id: trace_id, span_id: span_id) nrs = OpenTelemetry::Trace.non_recording_span(span_ctx) - corrupted_ctx = Instana::Trace.context_with_span(nrs) # OTel Context, not SpanContext + corrupted_ctx = Instana::Trace.context_with_span(nrs) # OTel Context, not SpanContext corrupted_span = OpenTelemetry::Trace.non_recording_span(corrupted_ctx) # .context => OTel Context leaked_frame = OpenTelemetry::Trace.context_with_span(corrupted_span) token = OpenTelemetry::Context.attach(leaked_frame) @@ -58,7 +58,7 @@ def with_corrupted_frame(trace_id: 'abc123', span_id: 'def456') # Assert that the span returned by start_span is a live, recording Instana span. def assert_recording_span(span, msg = nil) assert_instance_of Instana::Span, span, msg || 'Expected an Instana::Span' - assert span.recording?, "#{msg || 'Span'} must be recording" + assert span.recording?, "#{msg || 'Span'} must be recording" end # ----------------------------------------------------------------------- @@ -147,7 +147,7 @@ def test_net_http_skips_instrumentation_when_not_tracing_and_exit_as_root_is_off # `started?` is an instance method of Net::HTTP and is not testable here # without a live connection, so we omit it — it is not relevant to the # poisoned-context path being exercised. - dnt_spans = %i[dynamodb sqs sns s3] + dnt_spans = [:dynamodb, :sqs, :sns, :s3] skip = !Instana.tracer.tracing? || !Instana.config[:nethttp][:enabled] || (!::Instana.tracer.current_span.nil? && @@ -187,7 +187,7 @@ def test_grpc_start_span_tolerates_context_wrapping_otel_context_as_parent # upstream Instana headers are present. incoming_context = Instana::SpanContext.new( trace_id: ::Instana::Util.header_to_id('deadbeef'), - span_id: ::Instana::Util.header_to_id('cafebabe') + span_id: ::Instana::Util.header_to_id('cafebabe') ) nrs = OpenTelemetry::Trace.non_recording_span(incoming_context) parent_context = Instana::Trace.context_with_span(nrs) # OTel Context — the corrupted form