getsentry / getsentry/sentry-ruby

SessionFlusher#add_session races with the flush swap

Abierto
#3,038 1 comentario 0 reacciones 1 asignado Reclamado por @sl0thentr0py Ver en GitHub
Ruby
Lenguaje dominante
Ruby
Estrellas
987
Forks
541
Merge medio
17 h 40 min
PR fusionados (30 d)
19

Descripción

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

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.