getsentry / getsentry/sentry-ruby

SessionFlusher#add_session races with the flush swap

Aperta
#3,038 1 commento 0 reazioni 1 assegnatario Rivendicata da @sl0thentr0py Vedi su GitHub
Ruby
Lingua principale
Ruby
Stelle
987
Fork
541
Merge medio
17h 40m
PR unite (30g)
19

Descrizione

`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:

```ruby
@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.

```ruby
# 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
```

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.