rabbit_feature_flags: Inject test feature flags atomically

`rabbit_feature_flags:inject_test_feature_flags/2` could discard flags
from the persistent term because the read and write to the persistent
term were not atomic. `feature_flags_SUITE:registry_concurrent_reloads/1`
spawns many processes to modify the feature flag registry concurrently
but also updates this persistent term concurrently. The processes would
race so that many would read the initial flags, add their flag and write
that state, discarding any flags that had been written in the meantime.

We can add a lock around changes to the persistent term to make the
changes atomic.

Non-atomic updates to the persistent term caused unexpected behavior in
the `registry_concurrent_reloads/1` case previously. The
`PT_TESTSUITE_ATTRS` persistent_term only ended up containing a few of
the desired feature flags (for example only `ff_02` and `ff_06` along
with the base `ff_a` and `ff_b`). The case did not fail because the
registry continued to make progress towards that set of feature flags.
However the test case never reached the "all feature flags appeared"
state of the spammer and so the new assertion added at the end of the
case in this commit would fail.

deps/rabbit/src/rabbit_feature_flags.erl

@@ -772,11 +772,17 @@ init() ->
     ok.
 
 -define(PT_TESTSUITE_ATTRS, {?MODULE, testsuite_feature_flags_attrs}).
+%% We must lock while making updates to the above persistent_term in order to
+%% make the updates atomic. Otherwise if two processes attempt to inject
+%% different flags at the same time, they might race and a flag could be
+%% mistakenly discarded.
+-define(LOCK_TESTSUITE_ATTRS, {?PT_TESTSUITE_ATTRS, self()}).
 
 inject_test_feature_flags(FeatureFlags) ->
     inject_test_feature_flags(FeatureFlags, true).
 
 inject_test_feature_flags(FeatureFlags, InitReg) ->
+    true = global:set_lock(?LOCK_TESTSUITE_ATTRS, [node()]),
     ExistingAppAttrs = module_attributes_from_testsuite(),
     FeatureFlagsPerApp0 = lists:foldl(
                             fun({Origin, Origin, FFlags}, Acc) ->
@@ -810,13 +816,17 @@ inject_test_feature_flags(FeatureFlags, InitReg) ->
       [FeatureFlags, AttributesFromTestsuite],
       #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}),
     ok = persistent_term:put(?PT_TESTSUITE_ATTRS, AttributesFromTestsuite),
+    true = global:del_lock(?LOCK_TESTSUITE_ATTRS, [node()]),
     case InitReg of
         true  -> rabbit_ff_registry_factory:initialize_registry();
         false -> ok
     end.
 
 clear_injected_test_feature_flags() ->
-    _ = persistent_term:erase(?PT_TESTSUITE_ATTRS),
+    _ = global:trans(
+          ?LOCK_TESTSUITE_ATTRS,
+          fun() -> persistent_term:erase(?PT_TESTSUITE_ATTRS) end,
+          [node()]),
     ok.
 
 module_attributes_from_testsuite() ->

deps/rabbit/test/feature_flags_SUITE.erl

@@ -551,7 +551,9 @@ registry_concurrent_reloads(_Config) ->
     MRef = erlang:monitor(process, Spammer),
     unlink(Spammer),
     exit(Spammer, kill),
-    receive {'DOWN', MRef, process, Spammer, _} -> ok end.
+    receive {'DOWN', MRef, process, Spammer, _} -> ok end,
+    %% All feature flags appeared.
+    ?assertEqual(FinalFFList, ?list_ff(all)).
 
 registry_spammer(CurrentFeatureNames, FinalFeatureNames) ->
     %% Infinite loop.