Skip to content

SessionFlusher#add_session races with the flush swap #3038

Description

@SeanLF

add_session (session_flusher.rb:26-33) writes @pending_aggregates with no lock, while pending_envelope (:44-49) swaps in a fresh Hash under @mutex. Only the reader takes the mutex, so it excludes nothing.

add_session does two lookups:

@pending_aggregates[session.aggregation_key] ||= init_aggregates(session.aggregation_key)
@pending_aggregates[session.aggregation_key][session.status] += 1

If the swap lands between them, the second returns nil. Reproduced as NoMethodError: undefined method '[]' for nil, raised inside Hub#end_session, which runs in the Rack middleware's ensure.

Rare in production, since it needs the 60s tick to land in a narrow window. The spec below drives the swap from init_aggregates to make the interleaving deterministic. TelemetryEventBuffer locks both sides, which suggests this is an oversight rather than intent.

Failing spec

Drop in sentry-ruby/spec/ and run bundle exec rspec spec/session_flusher_race_spec.rb. Fails on master with NoMethodError: undefined method '[]' for nil, deterministically.

# Run from sentry-ruby/: bundle exec rspec path/to/session_flusher_race_spec.rb
require "spec_helper"

RSpec.describe "SessionFlusher#add_session vs the flush swap" do
  before { perform_basic_setup { |config| config.release = "1.0" } }

  let(:flusher) do
    Sentry::SessionFlusher.new(Sentry.configuration, Sentry.get_current_client).tap do |f|
      def f.ensure_thread = true # skip the real flush thread
    end
  end

  def exited_session
    Sentry::Session.new.tap { |s| s.instance_variable_set(:@status, :exited) }
  end

  # add_session reads @pending_aggregates twice with no lock:
  #
  #   @pending_aggregates[key] ||= init_aggregates(key)
  #   @pending_aggregates[key][status] += 1
  #
  # In production the flusher thread's 60s swap lands between them. Here the
  # swap is driven from init_aggregates to make that interleaving deterministic
  # rather than relying on a 1-in-20k stress race.
  it "does not raise when the pending hash is swapped mid-update" do
    allow(flusher).to receive(:init_aggregates).and_wrap_original do |original, *args|
      original.call(*args).tap { flusher.send(:pending_envelope) }
    end

    expect { flusher.add_session(exited_session) }.not_to raise_error
  end
end

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions