[Deferred] Propagate ActiveSupport::CurrentAttributes across fiber boundaries#280
Open
jsxs0 wants to merge 1 commit intorage-rb:mainfrom
Open
[Deferred] Propagate ActiveSupport::CurrentAttributes across fiber boundaries#280jsxs0 wants to merge 1 commit intorage-rb:mainfrom
jsxs0 wants to merge 1 commit intorage-rb:mainfrom
Conversation
5024ff4 to
b4b27ed
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Propagates
ActiveSupport::CurrentAttributesacrossRage::Deferred's fiber boundary so background tasks see the sameCurrent.*values as the request that enqueued them.Problem
While tracing log-context propagation in #267 (SSE) and #274 (Deferred), I kept ending up at the same place.
Rage::Deferred::Queue#schedulespawns a fresh fiber viaFiber.schedule(queue.rb:48), and everyFiber[:key]from the parent fiber gets wiped.Context.buildcatches logger tags (index 4) and logger context (index 5), butActiveSupport::CurrentAttributesgets silently dropped at exactly the same boundary. Which is surprising, because Rage explicitly wiresCurrentAttributesto fiber-local storage viaIsolatedExecutionState.isolation_level = :fiberatext/setup.rb.Concretely: set
Current.user,Current.tenant, orCurrent.request_idin a request, enqueue a deferred task, and the task runs with all of those nil. Apps relying onCurrentAttributesfor multi-tenancy, audit trails, or trace context break invisibly the moment work crosses into the queue.Approach
Extends the
Contextarray with index 7, storing a snapshot of eachActiveSupport::CurrentAttributessubclass's attributes at enqueue time. Values are.dup'd so a subsequentCurrent.resetin the parent fiber cannot mutate what was captured.In the task fiber,
__performreads the snapshot and restores values via direct writer calls. Theensureblock only resets subclasses that were actually restored. Resetting everyCurrentAttributesdescendant on every task completion would pointlessly firebefore_reset/after_resetcallbacks across the whole app. The same rescue-per-subclass pattern from the #248 connection-cleanup work applies, so one broken subclass cannot take down the whole task.Why snapshot-and-restore rather than patching
CurrentAttributes' internal storageI spent some time considering a version that patched AS internals so attribute writes routed through a dedicated
Fiber[:key], mirroring how Rage already treats logger tags. It would have been elegant. But monkey-patching AS internals is fragile across Rails versions and hostile to the next maintainer. Rails itself solves this exact problem with snapshot-and-restore inActiveJob::CurrentAttributes. If it's good enough for Rails' own background-job system, it's good enough here, and the code reads as a mirror of an established pattern rather than a novel hack.What this gets us:
Current.*values between tasksScope
Intentionally scoped to the
Rage::Deferredfiber boundary only. The same pattern applies toRage::SSE::Application#start_stream(sse/application.rb:60, which I was inside recently for #264 and #267) andRage::FiberScheduler#fiberfor user-spawned fibers. I'll open those as separate PRs so each can be reviewed on its own merits.Test plan
bundle exec rspec spec/deferred/: 146 examples, 0 failuresbundle exec rspec spec/telemetry/: 77 examples, 0 failures, 2 pending (pre-existing external-test skips, unrelated)bundle exec rspec spec/sse/: 79 examples, 0 failuresFive new examples on
Context.capture_current_attributes: AS not loaded, AS loaded with no subclasses, subclasses with attributes (capture + dup verification), empty-attribute subclasses. Five onTask#__perform: restore beforeperform, reset afterperform, reset onperformraise, graceful continuation when a subclass writer raises, and a no-op path when AS isn't loaded.Notes
CurrentAttributessubclasses with small attribute counts, but I want to measure before claiming a number. Will add in a follow-up commit.nilatget_current_attributesand no-op cleanly through restore/reset.Context
In my day job I run SSE systems for industrial IoT monitoring, which is how I ended up reading Rage's SSE and Deferred internals in the first place. The bug this fixes is one I've shipped around by hand in other frameworks, carrying
device_idandtenantthrough async hops manually because the framework wouldn't. Nice to be able to fix it at the right layer here.cc @rsamoilov