Skip to content

Commit ff6cec0

Browse files
jnunemakerclaude
andcommitted
Fix thread safety and robustness in Poll adapter and Sync middleware
- Add mutex to Poll#sync to prevent redundant concurrent imports - Use rescue StandardError instead of bare rescue in Poll and CloudPoll - Cache poll adapter lookup by identity in Sync middleware to avoid object_id reuse issues after GC - Document write-through eventual consistency trade-off Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 29a5122 commit ff6cec0

File tree

3 files changed

+29
-9
lines changed

3 files changed

+29
-9
lines changed

lib/flipper/adapters/cloud_poll.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ def initialize(poller, adapter)
2727
if adapter.features.empty?
2828
begin
2929
@poller.sync
30-
rescue
30+
rescue StandardError
3131
# TODO: Warn here that it's possible that no data has been synced
3232
# and flags are being evaluated without flag data being present
3333
# until a sync completes. We rescue to avoid flipper being down

lib/flipper/adapters/poll.rb

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ class Poll
1818
# Public: The Poller instance used to sync in the background.
1919
attr_reader :poller
2020

21+
# Public: The mutex used to synchronize sync operations.
22+
attr_reader :sync_mutex
23+
2124
# Public: The local memory adapter that serves reads.
2225
attr_reader :local
2326

@@ -46,13 +49,14 @@ def initialize(source, options = {})
4649
@local = Adapters::Memory.new
4750
@remote = source
4851
@last_synced_at = Concurrent::AtomicFixnum.new(0)
52+
@sync_mutex = Mutex.new
4953

5054
# Block the main thread for the initial sync so we don't serve
5155
# empty/default values before the first poll completes.
5256
begin
5357
@poller.sync
5458
sync
55-
rescue
59+
rescue StandardError
5660
# Rescue to avoid source adapter being down causing processes to crash.
5761
end
5862
end
@@ -67,11 +71,13 @@ def adapter_stack
6771
# If given a block, syncs once at the start and suppresses further syncs
6872
# for the duration of the block (useful for per-request sync).
6973
def sync
70-
poller_last_synced_at = @poller.last_synced_at.value
71-
last = @last_synced_at.value
72-
if poller_last_synced_at > last
73-
@local.import(@poller.adapter)
74-
@last_synced_at.update { poller_last_synced_at }
74+
@sync_mutex.synchronize do
75+
poller_last_synced_at = @poller.last_synced_at.value
76+
last = @last_synced_at.value
77+
if poller_last_synced_at > last
78+
@local.import(@poller.adapter)
79+
@last_synced_at.update { poller_last_synced_at }
80+
end
7581
end
7682

7783
if block_given?
@@ -106,7 +112,10 @@ def get_all(**kwargs)
106112
@local.get_all(**kwargs)
107113
end
108114

109-
# Writes - go to source first, then update local memory
115+
# Writes - go to source first, then update local memory.
116+
# Note: There is a small window where another thread's sync could overwrite
117+
# the local adapter with a stale poller snapshot that doesn't include the
118+
# write yet. The write will be picked up on the next poll cycle.
110119

111120
def add(feature)
112121
@remote.add(feature).tap { @local.add(feature) }

lib/flipper/middleware/sync.rb

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ def initialize(app, options = {})
88

99
def call(env)
1010
flipper = env.fetch(@env_key) { Flipper }
11-
poll_adapter = find_poll_adapter(flipper.adapter)
11+
poll_adapter = poll_adapter_for(flipper)
1212

1313
if poll_adapter
1414
poll_adapter.sync { @app.call(env) }
@@ -19,6 +19,17 @@ def call(env)
1919

2020
private
2121

22+
# Cache the poll adapter lookup since the adapter stack doesn't change
23+
# after initialization. Uses the flipper instance itself as key to avoid
24+
# object_id reuse issues after GC.
25+
def poll_adapter_for(flipper)
26+
@poll_adapters ||= {}.compare_by_identity
27+
unless @poll_adapters.key?(flipper)
28+
@poll_adapters[flipper] = find_poll_adapter(flipper.adapter)
29+
end
30+
@poll_adapters[flipper]
31+
end
32+
2233
# Walk the adapter stack to find a Poll adapter, which may be wrapped
2334
# by Strict, ActorLimit, or other Wrapper adapters.
2435
def find_poll_adapter(adapter)

0 commit comments

Comments
 (0)