Skip to content

Attribution is process-global, not thread-local — changes can be misattributed under concurrency #6

Description

@njakobsen

Root cause

lib/track_changes/attribution.rb:2:

mattr_accessor :default_attribution

That's a single @@default_attribution shared by every thread in the process. with_changes_attributed_to sets it, yields, and restores it in an ensure:

def self.with_changes_attributed_to(new_source, &block)
  old_source = self.default_attribution
  self.default_attribution = new_source
  block.call
ensure
  self.default_attribution = old_source
end

Model#track_changes_by falls back to it (lib/track_changes/model.rb:61), and attribute_changes_to (lib/track_changes/controller.rb) wraps the whole thing in an around_action.

So in a threaded server, the "current" attribution is process-wide, not per-request. Two concurrent requests interleave like this:

  1. Thread A (Alice) sets default_attribution = alice
  2. Thread B (Bob) sets default_attribution = bob
  3. Thread A saves a tracked record → reads default_attributionrecords the change as Bob's
  4. Thread B finishes, restores to alice; thread A finishes, restores to nil

This isn't theoretical for the consuming apps: stolo_connect runs Puma with 5 threads in a single process (config/puma.rb:8, workers commented out), and its referral/site/payment controllers all use attribute_changes_to :current_user. Any two overlapping requests from different users touching tracked records can cross-attribute.

The window is narrow (the misattribution has to land between another thread's set and restore) which is probably why it hasn't been noticed — but the failure is silent and writes a permanently wrong audit row. For a change-tracking gem specifically, "who made this change" being occasionally wrong is worse than it being absent, because the record looks authoritative.

The ensure block makes this worse in one respect: it restores to whatever old_source that thread captured, so threads can also clobber each other's restore, leaving attribution set to a stale user after a request completes.

Suggested resolution

Back the attribution with ActiveSupport::CurrentAttributes, which is per-thread (and per-fiber) and already resets between requests via the executor. Consuming apps are on Rails 8 and already use this idiom (Current.user in stolo_connect).

Rough shape:

module TrackChanges
  class Current < ActiveSupport::CurrentAttributes
    attribute :default_attribution
  end

  def self.default_attribution
    Current.default_attribution
  end

  def self.default_attribution=(value)
    Current.default_attribution = value
  end
end

That keeps with_changes_attributed_to and the mattr-style read/write API intact, so no consumer changes.

One constraint: ActiveSupport::CurrentAttributes arrived in Rails 5.2, but the gemspec declares rails >= 4.2.0 (track_changes.gemspec:20). Adopting it means raising that floor. Worth checking whether anything still consuming this gem is below 5.2 — if something is, the alternative is a plain Thread.current-keyed store, which works everywhere but doesn't get the executor's automatic reset between requests.

Worth a regression spec that runs two threads with different attributions concurrently and asserts each one's diff gets its own user.

Context

Found while scoping change tracking for guardian reports in stolo_connect.

Refs https://github.com/culturecode/stolo_connect/issues/TICKET1

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions