diff --git a/sentry-solid_queue/lib/sentry/solid_queue/active_job_extensions.rb b/sentry-solid_queue/lib/sentry/solid_queue/active_job_extensions.rb index 156d0284d..fe173e4aa 100644 --- a/sentry-solid_queue/lib/sentry/solid_queue/active_job_extensions.rb +++ b/sentry-solid_queue/lib/sentry/solid_queue/active_job_extensions.rb @@ -14,28 +14,36 @@ def perform_now job_executed = false - # Worker threads (deserialized from queue): clone hub for thread isolation, - # then operate directly on the scope (like Sidekiq's server middleware). - # No with_scope — avoids double Scope#dup overhead. - # - # Inline perform_now (e.g., from a controller): use with_scope to - # preserve the existing request scope/transaction. - if @_solid_queue_worker_thread - Sentry.clone_hub_to_current_thread - scope = Sentry.get_current_scope - perform_with_sentry(scope) { job_executed = true; super } - else - Sentry.with_scope do |scope| + # NOTE: the rescue below MUST stay scoped to this begin block. As a + # method-level `rescue` it also covered the guard clause above, where + # `job_executed` is still nil — so an exception raised by the guard's + # own `super` was swallowed and the job was performed a second time on + # the same instance (executions == 2, duplicated side effects, and the + # *second* run's error propagating in place of the real one). + begin + # Worker threads (deserialized from queue): clone hub for thread isolation, + # then operate directly on the scope (like Sidekiq's server middleware). + # No with_scope — avoids double Scope#dup overhead. + # + # Inline perform_now (e.g., from a controller): use with_scope to + # preserve the existing request scope/transaction. + if @_solid_queue_worker_thread + Sentry.clone_hub_to_current_thread + scope = Sentry.get_current_scope perform_with_sentry(scope) { job_executed = true; super } + else + Sentry.with_scope do |scope| + perform_with_sentry(scope) { job_executed = true; super } + end end + rescue => e + # If the job already executed, the exception is a re-raised job error — + # let it propagate. If not, Sentry setup failed before the job ran, + # so fall back to running the job without instrumentation. + raise if job_executed + Sentry.sdk_logger.error("sentry-solid_queue failed to instrument job: #{e.message}") rescue nil + super end - rescue => e - # If the job already executed, the exception is a re-raised job error — - # let it propagate. If not, Sentry setup failed before the job ran, - # so fall back to running the job without instrumentation. - raise if job_executed - Sentry.sdk_logger.error("sentry-solid_queue failed to instrument job: #{e.message}") rescue nil - super end # --- Client-side: inject trace headers on enqueue --- diff --git a/sentry-solid_queue/spec/sentry/solid_queue/active_job_extensions_spec.rb b/sentry-solid_queue/spec/sentry/solid_queue/active_job_extensions_spec.rb index 16b214ea9..4b15c14ae 100644 --- a/sentry-solid_queue/spec/sentry/solid_queue/active_job_extensions_spec.rb +++ b/sentry-solid_queue/spec/sentry/solid_queue/active_job_extensions_spec.rb @@ -11,6 +11,16 @@ result = HappyJob.perform_now expect(result).to eq("happy") end + + it "performs a raising job exactly once and propagates its own error" do + CountingSadJob.runs = 0 + job = CountingSadJob.new + + expect { job.perform_now }.to raise_error(RuntimeError, "counted failure") + + expect(CountingSadJob.runs).to eq(1) + expect(job.executions).to eq(1) + end end context "when Sentry is initialized without tracing" do @@ -24,6 +34,20 @@ expect(event.exception.values.first.type).to eq("RuntimeError") end + # The fully instrumented path: guard passes, so the job runs inside + # perform_with_sentry. A raising job must still be performed once, and be + # reported once — a duplicate run would also double the Sentry event. + it "performs a raising instrumented job exactly once and reports it once" do + CountingSadJob.runs = 0 + job = CountingSadJob.new + + expect { job.perform_now }.to raise_error(RuntimeError, "counted failure") + + expect(CountingSadJob.runs).to eq(1) + expect(job.executions).to eq(1) + expect(transport.events.count).to eq(1) + end + it "does not capture events for successful jobs" do HappyJob.perform_now @@ -44,6 +68,17 @@ expect(transport.events.count).to eq(0) end + it "performs a raising non-SolidQueue job exactly once and propagates its own error" do + CountingNonSolidQueueSadJob.runs = 0 + job = CountingNonSolidQueueSadJob.new + + expect { job.perform_now }.to raise_error(RuntimeError, "counted failure") + + expect(CountingNonSolidQueueSadJob.runs).to eq(1) + expect(job.executions).to eq(1) + expect(transport.events.count).to eq(0) + end + it "sets the mechanism to solid_queue" do expect { SadJob.perform_now }.to raise_error(RuntimeError) @@ -200,28 +235,42 @@ it "falls back to uninstrumented execution if Sentry errors before the job runs" do allow(Sentry).to receive(:clone_hub_to_current_thread).and_raise(StandardError, "sentry boom") + CountingHappyJob.runs = 0 - job = HappyJob.new + job = CountingHappyJob.new job_data = job.serialize job_data["_sentry"] = {} job.deserialize(job_data) # Should not raise — falls back to super result = job.perform_now + expect(result).to eq("happy") + # The fallback runs the job; it must not *re-run* one that already ran. + expect(CountingHappyJob.runs).to eq(1) + expect(job.executions).to eq(1) end it "re-raises job errors even when safety rescue is active" do - # The outer rescue must not swallow actual job exceptions - expect { simulate_worker_perform(SadJob) }.to raise_error(RuntimeError, "I'm sad!") + # The outer rescue must not swallow actual job exceptions, and must not + # mistake a job error for an instrumentation failure and retry the job. + CountingSadJob.runs = 0 + + expect { simulate_worker_perform(CountingSadJob) } + .to raise_error(RuntimeError, "counted failure") + + expect(CountingSadJob.runs).to eq(1) end it "falls back to uninstrumented execution on inline path if Sentry errors before the job runs" do allow(Sentry).to receive(:with_scope).and_raise(StandardError, "scope boom") + CountingHappyJob.runs = 0 # Inline path (no deserialization) — should fall back to super - result = HappyJob.perform_now + result = CountingHappyJob.perform_now + expect(result).to eq("happy") + expect(CountingHappyJob.runs).to eq(1) end end diff --git a/sentry-solid_queue/spec/spec_helper.rb b/sentry-solid_queue/spec/spec_helper.rb index f1979b00b..63a50f2e6 100644 --- a/sentry-solid_queue/spec/spec_helper.rb +++ b/sentry-solid_queue/spec/spec_helper.rb @@ -130,6 +130,60 @@ def perform end end +# Jobs that count their own executions. +# +# `perform_now` is a wrapper, so the invariant that matters is *how many times* +# it calls the wrapped job — not what the job returns. Asserting only on the +# return value or the error class is blind to a duplicate run: an idempotent +# job produces an identical outcome either way, which is how a method-level +# rescue managed to perform every job twice without failing a single example. +# Use these fixtures, and assert on `runs`, wherever a path could double up. +# +# CountingSadJob and CountingNonSolidQueueSadJob also cover the guard's two +# early-return paths — Sentry uninitialized, and a non-solid_queue adapter. +class CountingSadJob < ActiveJob::Base + self.queue_adapter = :solid_queue + + class << self + attr_accessor :runs + end + self.runs = 0 + + def perform + self.class.runs += 1 + raise "counted failure" + end +end + +class CountingNonSolidQueueSadJob < ActiveJob::Base + self.queue_adapter = :async + + class << self + attr_accessor :runs + end + self.runs = 0 + + def perform + self.class.runs += 1 + raise "counted failure" + end +end + +# Succeeds, so only the counter can tell one run from two. +class CountingHappyJob < ActiveJob::Base + self.queue_adapter = :solid_queue + + class << self + attr_accessor :runs + end + self.runs = 0 + + def perform + self.class.runs += 1 + "happy" + end +end + class RetryableJob < ActiveJob::Base self.queue_adapter = :solid_queue retry_on RuntimeError, wait: 0, attempts: 3