getsentry / getsentry/sentry-ruby
SessionFlusher#add_session races with the flush swap
- 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.