Skip to content

Commit 1ccd744

Browse files
pablomhclaude
andcommitted
Fixes #39277 - Fix TopbarSweeper thread-safety race condition
TopbarSweeper uses a Singleton with a shared controller attribute across all Puma threads. Under concurrent requests, one thread ensure block nils the controller while another thread is still using it, causing NoMethodError on expire_fragment. Replace the Singleton with Thread.current storage, following the same pattern used by ThreadSession for User.current/Organization.current. Each thread gets its own controller reference - no cross-thread clobbering. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 56d1031 commit 1ccd744

4 files changed

Lines changed: 45 additions & 10 deletions

File tree

app/controllers/concerns/foreman/controller/topbar_sweeper.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@ module TopbarSweeper
88
end
99

1010
def set_topbar_sweeper_controller
11-
::TopbarSweeper.instance.controller = self
11+
::TopbarSweeper.controller = self
1212
yield
1313
ensure
14-
::TopbarSweeper.instance.controller = nil
14+
::TopbarSweeper.controller = nil
1515
end
1616
end
1717
end

app/models/concerns/topbar_cache_expiry.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,6 @@ module TopbarCacheExpiry
88
end
99

1010
def expire_topbar_cache_within_controller
11-
expire_topbar_cache if TopbarSweeper.instance.controller.present?
11+
expire_topbar_cache if TopbarSweeper.controller
1212
end
1313
end

app/services/topbar_sweeper.rb

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,18 @@
11
class TopbarSweeper
2-
include Singleton
3-
attr_accessor :controller
4-
5-
def expire_cache(user = User.current)
6-
controller.expire_fragment(self.class.fragment_name(user.id)) if controller.present? && user.present?
7-
end
2+
THREAD_KEY = :topbar_sweeper_controller
83

94
class << self
10-
delegate :expire_cache, to: :instance
5+
def controller
6+
Thread.current[THREAD_KEY]
7+
end
8+
9+
def controller=(ctrl)
10+
Thread.current[THREAD_KEY] = ctrl
11+
end
12+
13+
def expire_cache(user = User.current)
14+
controller&.expire_fragment(fragment_name(user.id)) if user.present?
15+
end
1116

1217
def fragment_name(id = User.current.id)
1318
"tabs_and_title_records-#{id}"

test/unit/topbar_sweeper_test.rb

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
require 'test_helper'
2+
3+
class TopbarSweeperTest < ActiveSupport::TestCase
4+
def teardown
5+
TopbarSweeper.controller = nil
6+
end
7+
8+
test "controller is thread-local" do
9+
TopbarSweeper.controller = "main"
10+
thread_value = nil
11+
Thread.new { thread_value = TopbarSweeper.controller }.join
12+
assert_nil thread_value, "Other thread should not see main thread's controller"
13+
assert_equal "main", TopbarSweeper.controller
14+
end
15+
16+
test "expire_cache calls expire_fragment on controller" do
17+
mock_controller = Minitest::Mock.new
18+
user = FactoryBot.create(:user)
19+
mock_controller.expect :expire_fragment, nil, [TopbarSweeper.fragment_name(user.id)]
20+
TopbarSweeper.controller = mock_controller
21+
TopbarSweeper.expire_cache(user)
22+
mock_controller.verify
23+
end
24+
25+
test "expire_cache does not raise when controller is nil" do
26+
TopbarSweeper.controller = nil
27+
user = FactoryBot.create(:user)
28+
assert_nothing_raised { TopbarSweeper.expire_cache(user) }
29+
end
30+
end

0 commit comments

Comments
 (0)