From 814fd4df89505c2aff9b4c8c8cb9e96d44ba5a41 Mon Sep 17 00:00:00 2001 From: Bradley Priest Date: Sat, 17 Oct 2026 09:09:19 +1300 Subject: [PATCH 1/2] fix(rails): capture handled Rails.error reports during web requests The web request guard drops handled errors reported through Rails.error because they never reach the exception middleware. Deduplicate successful captures in the core client, including captured causes, so the Rails subscriber, middleware and ActiveJob integration can report the same exception safely. Remove their separate markers and the web request guard. --- .changeset/olive-cats-take.md | 10 +++ lib/posthog/client.rb | 28 +++++- posthog-rails/lib/posthog/rails.rb | 89 ------------------- posthog-rails/lib/posthog/rails/active_job.rb | 1 - .../lib/posthog/rails/capture_exceptions.rb | 9 +- .../lib/posthog/rails/error_subscriber.rb | 9 -- public_api_snapshot.txt | 1 - spec/posthog/client_spec.rb | 25 ++++++ .../posthog/rails/exception_mechanism_spec.rb | 73 +++++++++++---- 9 files changed, 119 insertions(+), 126 deletions(-) create mode 100644 .changeset/olive-cats-take.md diff --git a/.changeset/olive-cats-take.md b/.changeset/olive-cats-take.md new file mode 100644 index 00000000..8537bc70 --- /dev/null +++ b/.changeset/olive-cats-take.md @@ -0,0 +1,10 @@ +--- +"posthog-rails": patch +"posthog-ruby": patch +--- + +Capture handled errors reported through `Rails.error` during web requests by replacing Rails-specific suppression with shared exception deduplication. + +For mutable exceptions, `capture_exception` now suppresses subsequent captures of the same exception object and its captured causes, including manual calls and calls through another client. The first successful capture wins; subsequent calls return `false` without merging context. Supply the required identity and context on the first report. Failed captures remain retryable. + +Remove `PostHog::Rails::IN_WEB_REQUEST_KEY` and the request-tracking and exception-marker helpers documented as private. Remove any application references to these integration internals. diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index 17eae407..94d6b1c6 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -54,6 +54,9 @@ class Client RELEASE_ID_ENV_VAR = 'POSTHOG_RELEASE_ID' private_constant :RELEASE_ID_ENV_VAR + CAPTURED_EXCEPTION_IVAR = :@posthog_exception_captured + private_constant :CAPTURED_EXCEPTION_IVAR + # Thread-safe tracking of client instances per API key for singleton warnings @instances_by_api_key = {} @instances_mutex = Mutex.new @@ -409,7 +412,7 @@ def capture(attrs) # Defaults to `{ 'type' => 'generic', 'handled' => true }` for manual captures. # @return [Boolean, nil] Whether the exception event was queued or sent, or nil if the input could not be parsed. def capture_exception(exception, distinct_id = nil, additional_properties = {}, flags: nil, mechanism: nil) - return false if @disabled + return false if @disabled || exception_captured?(exception) exception_list = ExceptionCapture.build_exception_list(exception, mechanism: mechanism) @@ -426,7 +429,9 @@ def capture_exception(exception, distinct_id = nil, additional_properties = {}, } event_data[:flags] = flags if flags - capture(event_data) + capture(event_data).tap do |captured| + mark_exception_captured(exception) if captured + end end # Identifies a user @@ -977,6 +982,25 @@ def shutdown(timeout: nil) private + def exception_captured?(exception) + exception.is_a?(Exception) && exception.instance_variable_get(CAPTURED_EXCEPTION_IVAR) == true + rescue StandardError + false + end + + # Rails can unwrap template errors, so mark the causes included in the event too. + def mark_exception_captured(exception) + ExceptionCapture::MAX_CHAINED_EXCEPTIONS.times do + break unless exception.is_a?(Exception) + break if exception_captured?(exception) + + exception.instance_variable_set(CAPTURED_EXCEPTION_IVAR, true) unless exception.frozen? + exception = exception.cause + end + rescue StandardError + nil + end + def enrich_capture_attrs_with_context(attrs) context = Internal::Context.current explicit_properties = attrs[:properties] diff --git a/posthog-rails/lib/posthog/rails.rb b/posthog-rails/lib/posthog/rails.rb index 7a70f32d..36240784 100644 --- a/posthog-rails/lib/posthog/rails.rb +++ b/posthog-rails/lib/posthog/rails.rb @@ -18,14 +18,6 @@ module PostHog module Rails - # Thread-local key for tracking web request context - IN_WEB_REQUEST_KEY = :posthog_in_web_request - ACTIVE_JOB_CAPTURED_EXCEPTION_IVAR = :@posthog_active_job_exception_captured - WEB_CAPTURED_EXCEPTION_IVAR = :@posthog_web_exception_captured - - private_constant :ACTIVE_JOB_CAPTURED_EXCEPTION_IVAR - private_constant :WEB_CAPTURED_EXCEPTION_IVAR - class << self # @return [PostHog::Rails::Configuration] Rails integration configuration. def config @@ -42,87 +34,6 @@ def config def configure yield config if block_given? end - - # Mark that we're in a web request context - # CaptureExceptions middleware will handle exception capture - # @api private - # @return [void] - def enter_web_request - Thread.current[IN_WEB_REQUEST_KEY] = true - end - - # Clear web request context (called at end of request) - # @api private - # @return [void] - def exit_web_request - Thread.current[IN_WEB_REQUEST_KEY] = false - end - - # Check if we're currently in a web request context - # Used by ErrorSubscriber to avoid duplicate captures - # @api private - # @return [Boolean] - def in_web_request? - Thread.current[IN_WEB_REQUEST_KEY] == true - end - - # Mark an exception as already captured by the ActiveJob integration. - # Used by ErrorSubscriber to avoid duplicate captures when Rails reports - # the same re-raised job exception via Rails.error. - # @api private - # @param exception [Exception] - # @return [void] - def mark_active_job_exception_captured(exception) - exception.instance_variable_set(ACTIVE_JOB_CAPTURED_EXCEPTION_IVAR, true) - rescue StandardError - nil - end - - # Check whether an exception was already captured by ActiveJobExtensions. - # @api private - # @param exception [Exception] - # @return [Boolean] - def active_job_exception_captured?(exception) - exception.instance_variable_get(ACTIVE_JOB_CAPTURED_EXCEPTION_IVAR) == true - rescue StandardError - false - end - - # Mark an exception as already captured by the CaptureExceptions middleware. - # - # ActionDispatch::Executor sits above our middleware and reports unhandled - # exceptions to Rails.error *after* the response has unwound back through - # CaptureExceptions, so `in_web_request?` is already false by then. The mark - # lets ErrorSubscriber recognize the report as a duplicate. - # - # The whole cause chain is marked because ActionDispatch::ExceptionWrapper - # unwraps ActionView::Template::Error to its cause, meaning Rails can report - # a different object than the one the middleware captured. - # @api private - # @param exception [Exception] - # @return [void] - def mark_web_exception_captured(exception) - current = exception - seen = {}.compare_by_identity - - while current.is_a?(Exception) && !seen[current] - seen[current] = true - current.instance_variable_set(WEB_CAPTURED_EXCEPTION_IVAR, true) - current = current.cause - end - rescue StandardError - nil - end - - # Check whether an exception was already captured by CaptureExceptions. - # @api private - # @param exception [Exception] - # @return [Boolean] - def web_exception_captured?(exception) - exception.instance_variable_get(WEB_CAPTURED_EXCEPTION_IVAR) == true - rescue StandardError - false - end end end end diff --git a/posthog-rails/lib/posthog/rails/active_job.rb b/posthog-rails/lib/posthog/rails/active_job.rb index b621ae8e..a7842f88 100644 --- a/posthog-rails/lib/posthog/rails/active_job.rb +++ b/posthog-rails/lib/posthog/rails/active_job.rb @@ -70,7 +70,6 @@ def capture_job_exception(exception) properties, mechanism: { 'type' => 'active_job', 'handled' => false } ) - PostHog::Rails.mark_active_job_exception_captured(exception) rescue StandardError => e # Don't let PostHog errors break job processing PostHog::Logging.logger.error("Failed to capture job exception: #{e.message}") diff --git a/posthog-rails/lib/posthog/rails/capture_exceptions.rb b/posthog-rails/lib/posthog/rails/capture_exceptions.rb index e762a5ad..6cbd8657 100644 --- a/posthog-rails/lib/posthog/rails/capture_exceptions.rb +++ b/posthog-rails/lib/posthog/rails/capture_exceptions.rb @@ -18,10 +18,6 @@ def initialize(app) # @param env [Hash] Rack environment. # @return [Array] Rack response. def call(env) - # Signal that we're in a web request context - # ErrorSubscriber will skip capture for web requests to avoid duplicates - PostHog::Rails.enter_web_request - response = @app.call(env) env['posthog.response_status_code'] = response_status(response) @@ -35,8 +31,6 @@ def call(env) # Capture unhandled exceptions capture_exception(e, env) if should_capture?(e) raise - ensure - PostHog::Rails.exit_web_request end private @@ -60,13 +54,12 @@ def capture_exception(exception, env) distinct_id = extract_distinct_id(env) additional_properties = build_properties(request, env) - captured = PostHog.capture_exception( + PostHog.capture_exception( exception, distinct_id, additional_properties, mechanism: { 'type' => 'rails', 'handled' => false } ) - PostHog::Rails.mark_web_exception_captured(exception) if captured rescue StandardError => e PostHog::Logging.logger.error("Failed to capture exception: #{e.message}") PostHog::Logging.logger.error("Backtrace: #{e.backtrace&.first(5)&.join("\n")}") diff --git a/posthog-rails/lib/posthog/rails/error_subscriber.rb b/posthog-rails/lib/posthog/rails/error_subscriber.rb index a1f0890a..207a8790 100644 --- a/posthog-rails/lib/posthog/rails/error_subscriber.rb +++ b/posthog-rails/lib/posthog/rails/error_subscriber.rb @@ -20,15 +20,6 @@ class ErrorSubscriber def report(error, handled:, severity:, context:, source: nil) return unless PostHog::Rails.config&.auto_capture_exceptions return unless PostHog::Rails.config&.should_capture_exception?(error) - # Skip if in a web request - CaptureExceptions middleware will handle it - # with richer context (URL, params, controller, etc.) - return if PostHog::Rails.in_web_request? - # ActionDispatch::Executor reports the exception once the response has - # unwound past our middleware, so the flag above is already cleared. - # CaptureExceptions marks what it captured to catch that late report. - return if PostHog::Rails.web_exception_captured?(error) - return if PostHog::Rails.config&.auto_instrument_active_job && - PostHog::Rails.active_job_exception_captured?(error) distinct_id = context[:user_id] || context[:distinct_id] diff --git a/public_api_snapshot.txt b/public_api_snapshot.txt index 4930d38e..16f70bd6 100644 --- a/public_api_snapshot.txt +++ b/public_api_snapshot.txt @@ -327,7 +327,6 @@ instance_method PostHog::Rails::Configuration#use_tracing_headers=(arg0) instance_method PostHog::Rails::Configuration#user_id_method() instance_method PostHog::Rails::Configuration#user_id_method=(arg0) constant PostHog::Rails::Configuration::DEFAULT_LOGS_MAX_RECORDS_PER_MINUTE: Integer -constant PostHog::Rails::IN_WEB_REQUEST_KEY: Symbol class PostHog::Rails::InitConfig instance_method PostHog::Rails::InitConfig#api_key=(value) instance_method PostHog::Rails::InitConfig#before_send=(value) diff --git a/spec/posthog/client_spec.rb b/spec/posthog/client_spec.rb index b992eb74..07e2c229 100644 --- a/spec/posthog/client_spec.rb +++ b/spec/posthog/client_spec.rb @@ -2139,6 +2139,7 @@ def release raise StandardError, 'Test exception' rescue StandardError => e client.capture_exception(e, 'user-123', { 'user_agent' => 'Test Agent', 'request_id' => 'req-123' }) + expect(client.capture_exception(e)).to be(false) end message = client.dequeue_last_message @@ -2162,6 +2163,29 @@ def release expect(client.dequeue_last_message[:timestamp]).to eq('2024-07-16T04:00:00.123Z') end + it 'does not mark an exception when it could not be queued' do + error = StandardError.new('boom') + allow(client).to receive(:capture).and_return(false, true) + + expect(client.capture_exception(error)).to be(false) + expect(client.capture_exception(error)).to be(true) + expect(client.capture_exception(error)).to be(false) + end + + %i[instance_variable_get instance_variable_set].each do |operation| + it "still captures when the exception marker's #{operation} fails" do + error = StandardError.new('boom') + allow(error).to receive(operation).and_raise(StandardError, 'marker failed') + + expect(client.capture_exception(error)).to be(true) + expect(client.queued_messages).to eq(1) + end + end + + it 'can capture frozen exceptions' do + expect(client.capture_exception(StandardError.new('boom').freeze)).to be(true) + end + it 'captures the full cause chain outermost-first' do begin begin @@ -2171,6 +2195,7 @@ def release end rescue StandardError => e client.capture_exception(e, 'user-123') + expect(client.capture_exception(e.cause)).to be(false) end message = client.dequeue_last_message diff --git a/spec/posthog/rails/exception_mechanism_spec.rb b/spec/posthog/rails/exception_mechanism_spec.rb index c00ebb9c..556401e8 100644 --- a/spec/posthog/rails/exception_mechanism_spec.rb +++ b/spec/posthog/rails/exception_mechanism_spec.rb @@ -20,8 +20,39 @@ PostHog::Rails.config = previous_config end + let(:client) { PostHog::Client.new(api_key: API_KEY, test_mode: true) } + before do - allow(PostHog).to receive(:capture_exception) + allow(PostHog).to receive(:client).and_return(client) + allow(PostHog).to receive(:capture_exception).and_call_original + end + + describe 'Rails error reports during requests' do + let(:reporter) { ActiveSupport::ErrorReporter.new.tap { |r| r.subscribe(PostHog::Rails::ErrorSubscriber.new) } } + + it 'captures handled errors with the existing request and reporter context' do + app = lambda do |_env| + reporter.handle(context: { distinct_id: 'shop-123', shop_id: 123 }) { raise 'handled' } + [200, {}, []] + end + middleware = PostHog::Rails::RequestContext.new(PostHog::Rails::CaptureExceptions.new(app)) + middleware.call(Rack::MockRequest.env_for('/orders')) + + expect(client.queued_messages).to eq(1) + event = client.dequeue_last_message + expect(event[:distinct_id]).to eq('shop-123') + expect(event[:properties]).to include('$request_path' => '/orders', '$context_shop_id' => 123) + expect(event[:properties]['$exception_list'].first['mechanism']['handled']).to be(true) + end + + it 'does not duplicate an error reported before it escapes the request' do + app = ->(_env) { reporter.record { raise 'unhandled' } } + middleware = PostHog::Rails::CaptureExceptions.new(app) + + expect { middleware.call(Rack::MockRequest.env_for('/orders')) }.to raise_error('unhandled') + expect(client.queued_messages).to eq(1) + expect(client.dequeue_last_message[:properties]['$exception_list'].first['mechanism']['handled']).to be(false) + end end describe PostHog::Rails::CaptureExceptions do @@ -41,7 +72,6 @@ end it 'prevents the Rails error subscriber from capturing the same web exception again' do - allow(PostHog).to receive(:capture_exception).and_return(true) app = ->(_env) { raise StandardError, 'boom' } middleware = described_class.new(app) env = Rack::MockRequest.env_for('/api/test') @@ -53,11 +83,6 @@ error = e end - # ActionDispatch::Executor sits above this middleware and reports to - # Rails.error only after the response has unwound past it, so the - # in-web-request flag is already cleared by the time the report lands. - expect(PostHog::Rails.in_web_request?).to be(false) - PostHog::Rails::ErrorSubscriber.new.report( error, handled: false, @@ -66,7 +91,7 @@ source: 'application.action_dispatch' ) - expect(PostHog).to have_received(:capture_exception).once + expect(client.queued_messages).to eq(1) expect(PostHog).to have_received(:capture_exception).with( an_instance_of(StandardError), anything, @@ -131,7 +156,7 @@ it 'skips exceptions already captured by ActiveJobExtensions' do PostHog::Rails.config.auto_instrument_active_job = true error = StandardError.new('boom') - PostHog::Rails.mark_active_job_exception_captured(error) + client.capture_exception(error) described_class.new.report( error, @@ -141,12 +166,12 @@ source: 'application.active_support' ) - expect(PostHog).not_to have_received(:capture_exception) + expect(client.queued_messages).to eq(1) end it 'skips exceptions already captured by CaptureExceptions' do error = StandardError.new('boom') - PostHog::Rails.mark_web_exception_captured(error) + client.capture_exception(error) described_class.new.report( error, @@ -156,7 +181,7 @@ source: 'application.action_dispatch' ) - expect(PostHog).not_to have_received(:capture_exception) + expect(client.queued_messages).to eq(1) end it 'skips the unwrapped cause of an exception already captured by CaptureExceptions' do @@ -170,7 +195,7 @@ end end - PostHog::Rails.mark_web_exception_captured(wrapped) + client.capture_exception(wrapped) # ActionDispatch::ExceptionWrapper unwraps ActionView::Template::Error to # its cause, so Rails reports a different object than the one captured. @@ -182,7 +207,7 @@ source: 'application.action_dispatch' ) - expect(PostHog).not_to have_received(:capture_exception) + expect(client.queued_messages).to eq(1) end it 'still captures unmarked ActiveSupport reports when ActiveJob instrumentation is enabled' do @@ -256,7 +281,6 @@ def perform_now it 'prevents Rails error subscriber from capturing the same job exception again' do PostHog::Rails.config.auto_instrument_active_job = true - allow(PostHog).to receive(:capture_exception).and_return(true) error = nil begin @@ -275,7 +299,8 @@ def perform_now source: 'application.active_support' ) - expect(PostHog).to have_received(:capture_exception).once + expect(client.queued_messages).to eq(1) + expect(client.dequeue_last_message[:properties]).to include('$job_class' => 'FakeJob', '$job_id' => 'job-1') expect(PostHog).to have_received(:capture_exception).with( an_instance_of(StandardError), anything, @@ -283,5 +308,21 @@ def perform_now mechanism: { 'type' => 'active_job', 'handled' => false } ) end + + it 'allows a Rails report to retry when the job event was dropped' do + PostHog::Rails.config.auto_instrument_active_job = true + attempts = 0 + retry_client = PostHog::Client.new(api_key: API_KEY, test_mode: true, before_send: lambda { |event| + attempts += 1 + event if attempts > 1 + }) + allow(PostHog).to receive(:client).and_return(retry_client) + + expect { job_class.new.perform_now }.to raise_error(StandardError, 'job failed') do |error| + PostHog::Rails::ErrorSubscriber.new.report(error, handled: false, severity: :error, context: {}) + end + + expect(retry_client.queued_messages).to eq(1) + end end end From e6392f0474493e97742e4e344341c1022af5323b Mon Sep 17 00:00:00 2001 From: Bradley Priest Date: Fri, 23 Oct 2026 14:52:37 +1300 Subject: [PATCH 2/2] fix: preserve exception retries and distinguish copied errors --- lib/posthog/client.rb | 17 ++++++----- spec/posthog/client_spec.rb | 56 +++++++++++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index 94d6b1c6..a898e95a 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -983,7 +983,7 @@ def shutdown(timeout: nil) private def exception_captured?(exception) - exception.is_a?(Exception) && exception.instance_variable_get(CAPTURED_EXCEPTION_IVAR) == true + exception.is_a?(Exception) && exception.instance_variable_get(CAPTURED_EXCEPTION_IVAR).equal?(exception) rescue StandardError false end @@ -994,7 +994,7 @@ def mark_exception_captured(exception) break unless exception.is_a?(Exception) break if exception_captured?(exception) - exception.instance_variable_set(CAPTURED_EXCEPTION_IVAR, true) unless exception.frozen? + exception.instance_variable_set(CAPTURED_EXCEPTION_IVAR, exception) unless exception.frozen? exception = exception.cause end rescue StandardError @@ -1175,10 +1175,7 @@ def enqueue(action) action = process_before_send(action) return false if action.nil? || action.empty? - if @sync_mode - send_sync(action) - return true - end + return send_sync(action) if @sync_mode queued = @queue_mutex.synchronize do if @queue.length < @max_queue_size @@ -1246,13 +1243,15 @@ def send_sync(action) batch << action rescue MessageBatch::JSONGenerationError => e @on_error.call(-1, e.to_s) - return + return false end - return if batch.empty? + return false if batch.empty? @sync_lock.synchronize do res = @transport.send(@api_key, batch) - @on_error.call(res.status, res.error) unless res.status.between?(200, 299) + success = res.status.between?(200, 299) + @on_error.call(res.status, res.error) unless success + success end end diff --git a/spec/posthog/client_spec.rb b/spec/posthog/client_spec.rb index 07e2c229..ab446e98 100644 --- a/spec/posthog/client_spec.rb +++ b/spec/posthog/client_spec.rb @@ -274,11 +274,36 @@ def shutdown ) sync_client = Client.new(api_key: API_KEY, sync_mode: true, on_error: on_error) - sync_client.capture(Queued::CAPTURE) + expect(sync_client.capture(Queued::CAPTURE)).to be(false) expect(error_status).to eq(400) end + it 'allows an exception to be retried after a failed synchronous request' do + PostHog::Transport.stub = false + request = stub_request(:post, 'https://us.i.posthog.com/batch/').to_return(status: 400).then + .to_return(status: 200) + on_error = proc {} + expect(on_error).to receive(:call).with(400, anything).once + sync_client = Client.new(api_key: API_KEY, sync_mode: true, on_error: on_error) + error = StandardError.new('boom') + + expect(sync_client.capture_exception(error)).to be(false) + expect(sync_client.capture_exception(error)).to be(true) + expect(sync_client.capture_exception(error)).to be(false) + expect(request).to have_been_requested.twice + end + + it 'allows an exception to be retried after an oversized event was dropped' do + sync_client = Client.new(api_key: API_KEY, sync_mode: true) + error = StandardError.new('boom') + properties = { details: 'x' * PostHog::Defaults::Message::MAX_BYTES } + + expect(sync_client.capture_exception(error, nil, properties)).to be(false) + expect(sync_client.capture_exception(error)).to be(true) + expect(sync_client.capture_exception(error)).to be(false) + end + [201, 202, 204].each do |status| it "does not call on_error for a #{status} response" do on_error = proc {} @@ -289,7 +314,7 @@ def shutdown ) sync_client = Client.new(api_key: API_KEY, sync_mode: true, on_error: on_error) - sync_client.capture(Queued::CAPTURE) + expect(sync_client.capture(Queued::CAPTURE)).to be(true) end end @@ -327,7 +352,7 @@ def shutdown ) sync_client = Client.new(api_key: API_KEY, sync_mode: true, on_error: on_error) - sync_client.capture(Queued::CAPTURE) + expect(sync_client.capture(Queued::CAPTURE)).to be(false) expect(error_status).to eq(-1) expect(error_message).to include('Serialization error') @@ -2172,6 +2197,31 @@ def release expect(client.capture_exception(error)).to be(false) end + %i[dup clone].each do |operation| + it "captures a distinct exception created with #{operation}" do + error = StandardError.new('boom') + expect(client.capture_exception(error)).to be(true) + + copy = error.public_send(operation) + expect(client.capture_exception(copy)).to be(true) + expect(client.capture_exception(copy)).to be(false) + expect(client.queued_messages).to eq(2) + end + end + + it 'captures a replacement exception raised with a different message' do + error = StandardError.new('original') + client.capture_exception(error) + + expect { raise error, 'different failure' }.to raise_error('different failure') do |replacement| + expect(client.capture_exception(replacement)).to be(true) + end + + expect(client.queued_messages).to eq(2) + expect(client.dequeue_last_message[:properties]['$exception_list'].first['value']).to eq('original') + expect(client.dequeue_last_message[:properties]['$exception_list'].first['value']).to eq('different failure') + end + %i[instance_variable_get instance_variable_set].each do |operation| it "still captures when the exception marker's #{operation} fails" do error = StandardError.new('boom')