From 0764709095427973d1089674c662667e24c8b3ee Mon Sep 17 00:00:00 2001 From: antoinefink Date: Thu, 6 Aug 2026 13:20:28 +0200 Subject: [PATCH 1/2] fix(solid_queue): don't perform the job twice when the guard clause raises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ActiveJobExtensions#perform_now` used a method-level `rescue`, which also covered its own guard clause: return super unless Sentry.initialized? && using_solid_queue_adapter? On that early-return path `job_executed` is still nil — Ruby registers the local from the lexical position of its assignment, so the rescue reads it as nil rather than raising NameError. `raise if job_executed` therefore did not fire: the original exception was swallowed and the fallback `super` performed the job a *second time on the same instance*. Observable whenever the guard is false (any non-solid_queue adapter, or Sentry not initialized): - `executions` reaches 2 for a single invocation - side effects run twice — duplicate enqueues, doubled API calls and logs - the error that finally propagates comes from the *second* run, masking the real one; an ActiveJob::Continuable job raises InvalidStepError on the replay because its step state was already recorded This hits every ActiveJob test in a host app, since the test adapter reports `queue_adapter_name == "test"`, and any environment using a different adapter (e.g. `:inline` on staging). Production on solid_queue was unaffected. Scope the rescue to a `begin` block around the instrumented section only, so the guard's own `super` is no longer covered by it. Adds regression coverage for both early-return paths — Sentry uninitialized, and a non-solid_queue adapter — asserting that a raising job is performed exactly once and propagates its own error. Both new examples fail on the old code with `expected: 1, got: 2`. Co-Authored-By: Claude Opus 5 --- .../solid_queue/active_job_extensions.rb | 46 +++++++++++-------- .../solid_queue/active_job_extensions_spec.rb | 21 +++++++++ sentry-solid_queue/spec/spec_helper.rb | 33 +++++++++++++ 3 files changed, 81 insertions(+), 19 deletions(-) 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..2c1605427 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 @@ -44,6 +54,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) diff --git a/sentry-solid_queue/spec/spec_helper.rb b/sentry-solid_queue/spec/spec_helper.rb index f1979b00b..702158237 100644 --- a/sentry-solid_queue/spec/spec_helper.rb +++ b/sentry-solid_queue/spec/spec_helper.rb @@ -130,6 +130,39 @@ def perform end end +# Raising jobs that count their own executions. Both take the `perform_now` +# guard's early-return path — CountingSadJob when Sentry is uninitialized, +# CountingNonSolidQueueSadJob because its adapter isn't solid_queue — which is +# where a method-level rescue used to swallow the error and perform the job a +# second time on the same instance. +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 + class RetryableJob < ActiveJob::Base self.queue_adapter = :solid_queue retry_on RuntimeError, wait: 0, attempts: 3 From fd89f38e6fed0216f79fd1119a0795cdd1790e35 Mon Sep 17 00:00:00 2001 From: antoinefink Date: Tue, 11 Aug 2026 17:43:32 +0200 Subject: [PATCH 2/2] test(solid_queue): assert execution counts, not just outcomes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `perform_now` is a wrapper, so the invariant that matters is how many times it calls the wrapped job. The suite asserted almost entirely on outcomes — return value, error class — which is structurally blind to a duplicate run: an idempotent job produces an identical outcome either way. That is how the method-level rescue performed every job twice without failing any of the 62 examples. Adds counting fixtures and converts the blind assertions: - the two "safety rescue" fallback examples used HappyJob and checked only `result == "happy"`; they now use CountingHappyJob and assert runs == 1, so a fallback that re-runs an already-executed job fails - "re-raises job errors even when safety rescue is active" checked only the error class; it now asserts the job ran once, so mistaking a job error for an instrumentation failure fails - adds a fully instrumented raising case (guard passes) asserting one run, executions == 1, and exactly one Sentry event These new assertions pass with and without the perform_now fix — the guard-true paths were never affected by that bug. They are forward-looking guards: a future change that breaks `job_executed` on the normal path would otherwise go unnoticed, exactly as the original defect did. Co-Authored-By: Claude Opus 5 --- .../solid_queue/active_job_extensions_spec.rb | 36 ++++++++++++++++--- sentry-solid_queue/spec/spec_helper.rb | 31 +++++++++++++--- 2 files changed, 58 insertions(+), 9 deletions(-) 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 2c1605427..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 @@ -34,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 @@ -221,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 702158237..63a50f2e6 100644 --- a/sentry-solid_queue/spec/spec_helper.rb +++ b/sentry-solid_queue/spec/spec_helper.rb @@ -130,11 +130,17 @@ def perform end end -# Raising jobs that count their own executions. Both take the `perform_now` -# guard's early-return path — CountingSadJob when Sentry is uninitialized, -# CountingNonSolidQueueSadJob because its adapter isn't solid_queue — which is -# where a method-level rescue used to swallow the error and perform the job a -# second time on the same instance. +# 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 @@ -163,6 +169,21 @@ def perform 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