feat: add config.hub_isolation_level for fiber-safe hub storage (Falcon/async)#3018
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5f89c7f. Configure here.
|
Both findings addressed in 3ce928c:
|
|
thanks a lot @SeanLF, if you don't mind, I will reduce the code a bit and make it more minimal. |
The current hub is stored in thread-local storage. On a fiber-based server (Falcon/async) many concurrent requests run as sibling fibers on one thread, so they share a single hub. When a request holds a scope across a reactor yield (streaming, IO), a concurrent request's with_scope mutates the same scope stack and scope/transaction/breadcrumbs/user leak between requests -- reproduced as 5/6 events attributed to the wrong user, including their email (cross-user PII bleed). Add an opt-in config.isolation_level (:thread default, :fiber) that routes the current hub through Sentry::HubStorage. :fiber uses Ruby 3.2+ Fiber Storage (Fiber[]), which gives per-request isolation AND child-fiber inheritance, so it fixes Falcon without re-introducing the graphql-ruby context loss that getsentry#1374/getsentry#1380 addressed (old fiber-local vars did not inherit; Fiber Storage does). Requesting :fiber on Ruby < 3.2 warns and falls back to :thread. Default :thread is byte-for-byte the previous behaviour. Mirrors Rails' config.active_support.isolation_level. Refs getsentry#1495. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The ActiveJob integration saved and restored the surrounding hub with Thread.current.thread_variable_get/set(Sentry::THREAD_LOCAL) directly, which is wrong under config.isolation_level = :fiber (it would read/write thread storage while the active hub lives in fiber storage). Route both sites through Sentry::HubStorage so the save/restore follows the configured isolation level. Behaviour is byte-for-byte unchanged under the default :thread level. Verified with a real ActiveJob perform_now: under :thread the outer hub is restored and the job's user does not leak; under :fiber, concurrent jobs run as sibling fibers each keep their own user. Refs getsentry#1495. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…pecs Two follow-ups from review: - Configuration#isolation_level= now applies the level to HubStorage when the SDK is already initialized, so changing Sentry.configuration.isolation_level after Sentry.init takes effect instead of leaving the SDK on the old backend. The default is assigned to the ivar directly in initialize so a throwaway Configuration.new cannot clobber the active isolation level (only Sentry.init builds the live config). - Make the :fiber isolation_level specs deterministic across Ruby versions by stubbing fiber_storage_available?, and add an explicit downgrade-to-:thread spec. Previously these asserted :fiber unconditionally and would fail on the Ruby < 3.2 CI cells, where :fiber correctly falls back to :thread. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
3ce928c to
ab54490
Compare
7c2e7f7 to
de42598
Compare
de42598 to
07e1f80
Compare
sl0thentr0py
left a comment
There was a problem hiding this comment.
- renamed config to
hub_isolation_levelfor more clarity - moved logic all inline to
Configurationinstead ofHubStorage
solnic
left a comment
There was a problem hiding this comment.
Thank you for working on this 🙏🏻 Left some nits for your consideration 😄
| if level == :fiber && !fiber_storage_available? | ||
| log_warn("hub_isolation_level :fiber requires Ruby 3.2+ Fiber Storage; falling back to :thread on Ruby #{RUBY_VERSION}.") | ||
| level = :thread | ||
| end |
There was a problem hiding this comment.
We could completely avoid that and define writer method dynamically depending on the availability, which would simplify runtime production code path.
There was a problem hiding this comment.
we still want to have the warning message if they try to set it, so this needs to stay
| # CaptureExceptions, or a stale hub left by a recycled thread-pool | ||
| # thread) so the outer context continues working correctly after | ||
| # the job finishes. | ||
| original_hub = Thread.current.thread_variable_get(Sentry::THREAD_LOCAL) |
There was a problem hiding this comment.
Was this comment inaccurate and that's why it got removed?
There was a problem hiding this comment.
we should generally not have this clanker generated comment bloat, so I will remove it wherever I see it unless it really adds any value.
| end | ||
|
|
||
| def hub_isolation_level=(level) | ||
| level = level.to_sym if level.respond_to?(:to_sym) |
There was a problem hiding this comment.
What's the reason for allowing non-symbols over here?
| attr_reader :hub_isolation_level | ||
|
|
||
| # Isolation levels the SDK understands for hub storage. | ||
| ISOLATION_LEVELS = %i[thread fiber].freeze |
There was a problem hiding this comment.
Nit and could be a follow-up mini-refactor, but: we have a validate macro - we could extend it to support included_in: [...] and then overriding the writer would only be needed for the situation where the Fiber storage API is not available.
|
Hello! I ran into this PR because I was having an issue in an app I'm running with Falcon that was leaking Sentry tags across requests where it should have been impossible. I ran this all through Claude Fable, so please accept my apologies, I'll try to keep the AI slop to a minimum, but I think we might have room to improve. I think the tl;dr is a bit strange because of my currently-mixed workload where I'm running Falcon for web, but SolidQueue for jobs, and SolidQueue doesn't ship with an Async/Fiber mode yet (but hopefully will soon: rails/solid_queue#728) So... Claude slop follows, edited a bit to keep it short, I hope this is helpful, but please accept my apologies for not entirely working through this myself in detail -- this should be taken with a grain of salt, but I thought it worth sharing: A caveat worth considering before this ships: Fiber storage is inherited by While evaluating the Why this bites in practiceIn a typical Rails app, hubs get seeded early and implicitly — e.g.
So a mixed web+jobs app that enables Repro sketch (Ruby 3.4, semantics-level)Fiber[:hub] = Object.new
threads = Array.new(4) { Thread.new { Fiber[:hub] } }
threads.map(&:value).map(&:object_id).uniq.size # => 1 — all threads share the fiber's objectWith hub accessors reading Possible mitigationTagging the stored hub with its owning thread restores per-thread cloning at thread boundaries while keeping fiber inheritance within a thread: def get_current_hub
thread, hub = Fiber[KEY]
thread.equal?(Thread.current) ? hub : clone_hub_to_current_thread
end
def clone_hub_to_current_thread
hub = get_main_hub.clone
Fiber[KEY] = [Thread.current, hub]
hub
end |
|
@trevorturk thanks for digging into this! My own claude managed to reproduce it on master
The existing regression spec missed it because it calls Two notes on your mitigation. Tagging with Cloning the hub is also not enough on its own. PR incoming. This is unreleased, so it can land before the next release |
|
Excellent! Thank you for digging into that. Please link the PR here so we can keep track and I'll QA IRL when everything is settled. |
|
PR is up: #3036 It tags each stored entry with its owning fiber, so a context that inherited one takes its own copy. The copy keeps the live span, otherwise child spans land on a detached transaction copy and are never sent. One thing that turned up while testing across the matrix, relevant to your Solid Queue setup: the Happy to have it QA'd IRL once it lands. |

What
Adds an opt-in
config.hub_isolation_level = :thread | :fiber(default:thread) that controls where the SDK stores the currentHub.:thread(default) — thread-local storage, exactly today's behaviour. Nothing changes for anyone who doesn't opt in.:fiber— Ruby 3.2+ Fiber Storage (Fiber[]). Each fiber gets its own hub, and child fibers inherit it, so concurrent requests on a fiber-based server (Falcon,async) are isolated instead of sharing and corrupting one another's scope.This is the fiber-based option discussed and deferred in #1495.
Why
On a fiber server, concurrent requests run as sibling fibers on one thread, so they share the single thread-local hub. When a request holds a scope across a reactor yield (streaming, IO), a concurrent request's
with_scopemutates the same scope stack, and scope/breadcrumbs/user leak between requests.Reproduced against a real
Async::HTTP::Server+ the realSentry::Rack::CaptureExceptionsmiddleware, firing concurrent HTTP requests that each set a user, yield mid-scope, then capture an event:Under thread-local, 5 of 6 events were reported under the wrong user (cross-user PII bleed).
:fiberfixes it.Why this is safe now, when #1380 moved the other way
#1380 (2021) moved off Ruby's old fiber-local variables (
Thread.current[]) because they don't inherit into child fibers, which lost graphql-ruby's context (#1374). Ruby 3.2 added Fiber Storage (Fiber#storage) — a different primitive that copies the parent's storage into a newly created fiber. It gives per-request isolation and child-fiber inheritance at once, so it fixes fiber servers without re-breaking #1374. The added regression tests prove both hold.This is also how the other SDKs already work (sentry-python
contextvars, sentry-javascriptAsyncLocalStorage— both isolate per task and inherit into children), and it follows opentelemetry-ruby's 2025 move off thread-local storage (#1807).Design
Sentry::HubStorageowns get/set/clear of the current hub and switches on the isolation level.THREAD_LOCAL = :sentry_hubis kept as the storage key (user monkey-patches reference the constant).config.isolation_levelis applied atSentry.init. Requesting:fiberon Ruby < 3.2 logs a warning and falls back to:thread(feature-detected, so nothing breaks on 2.7–3.1).thread_variable_*site now routes throughHubStorage(coresentry-ruby.rb, plussentry-railsactive_job.rband coretest_helper.rb, which touchedTHREAD_LOCALdirectly and would be wrong under:fiber).Mirrors Rails'
config.active_support.isolation_levelergonomics; usesFiber[](not Rails' non-inheritingFiber.attr_accessor) because the SDK needs the inheritance property.Compatibility
:threadis byte-for-byte the previous behaviour. Puma/Sidekiq/Unicorn/Resque are unaffected.>= 2.7still supported;:fiberrequires 3.2+ and degrades to:threadwith a warning below that.:fiberis safe on threaded servers too (each request re-clones the hub); the difference only matters when child fibers are spawned per request.Caveat (documented)
Under
:fiber, a rawThread.newspawned mid-request inherits the request's hub object (documented Ruby fiber-storage thread inheritance). CallSentry.clone_hub_to_current_threadin the spawned thread to isolate it — same as today.Tests
spec/sentry/hub_storage_spec.rb,config.isolation_levelspecs, and a fiber-isolation regression inspec/sentry_spec.rb(sibling fibers keep isolated user attribution across a yield; child fiber inherits the parent hub).:fiberspecs guard on Fiber-storage availability.Performance
benchmark-ips (Ruby 4.0.5):
Fiber[]get is 1.24x faster thanthread_variable_get; at the level of a fullSentry.add_breadcrumbthe two isolation levels are within measurement error. Not a hot-path regression.Notes for reviewers
sentry-railsfollow-on so ActiveJob is correct under:fiber. Happy to split the rails commit into a separate PR if you'd prefer to keep this core-only.CHANGELOG.mdedit (per the repo's auto-generated-at-release convention). Suggested entry:Refs #1495. /cc @sl0thentr0py @solnic
Changelog Entry
Add Fiber support new
config.hub_isolation_levelwhich can be set to:fiberor:thread. Use this in case you use Fiber based servers like Falcon