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

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