Fixes #39208 - Cache global registration script in smart-proxy

GET /register returns the same shell script for all hosts sharing the
same registration parameters (org, location, hostgroup, activation keys).
Under bulk registration — 100+ hosts hitting the same capsule simultaneously
— this endpoint is called once per host, each time proxying to Foreman and
waiting for the ERB template to render (~103ms in profiling).

Cache the rendered script in memory (5-minute TTL) keyed on a canonical
form of the request query string. The cache key is computed by parsing the
query string, sorting parameters alphabetically, and rebuilding — so
requests that differ only in parameter order (e.g. subscription-manager
and different client versions) share the same cache entry.

Per-key double-checked locking prevents the thundering herd on a cold
cache while allowing concurrent requests for genuinely different keys
(e.g. different activation keys) to fetch from Foreman in parallel:

  fast path  — unsynchronised Concurrent::Map read, no lock acquired
  slow path  — key-specific Mutex serialises only competing threads;
               double-check under lock before fetching from Foreman

The per-key Mutex is evicted from KEY_MUTEXES immediately after the
script is written to the cache. Once a key is hot, all future requests
take the fast path and KEY_MUTEXES is empty under steady state.

Only 2xx responses are cached — errors never poison the cache, so a
transient Foreman failure causes no lasting impact on subsequent requests.
Errors do not evict the mutex, so the next retry takes the slow path and
re-attempts the Foreman call.

SCRIPT_CACHE uses Concurrent::Map (already a smart-proxy dependency)
rather than a plain Hash. This provides lock-free reads that are safe on
all Ruby VMs without relying on MRI's GIL.

KEY_MUTEXES, KEY_MUTEXES_LOCK, and SCRIPT_CACHE are class-level constants
initialised at load time, avoiding the initialisation races that lazy ||=
patterns introduce in multi-threaded environments.

Tests added:
- Cache hit: Foreman called once for repeated identical requests
- Per-key isolation: different parameter sets cached independently
- Parameter order independence: requests differing only in param order
  share the same cache entry
- TTL expiry: expired entries are not served; Foreman is re-called
- Mutex eviction: KEY_MUTEXES is empty after a successful cache write
- Error non-caching: Foreman is called on every request when it errors
- setup clears both SCRIPT_CACHE and KEY_MUTEXES between tests

Author: pablomh

modules/registration/registration_api.rb

@@ -1,9 +1,55 @@
 require 'registration/proxy_request'
 
 class Proxy::Registration::Api < ::Sinatra::Base
+  # Cache for the global registration script (GET /register).
+  #
+  # The script is identical for all hosts sharing the same registration
+  # parameters (org, location, hostgroup, activation keys), making it safe
+  # to serve from an in-memory cache during concurrent bulk registration.
+  #
+  # Per-key double-checked locking prevents thundering herd while allowing
+  # genuinely independent cache keys (e.g. different activation keys) to
+  # fetch from Foreman in parallel. Only threads competing for the same key
+  # are serialized — one fetches while the rest block for ~103ms then receive
+  # the cached result immediately. Threads for different keys are unaffected.
+  #
+  # Only successful (2xx) responses are cached — errors never poison the cache.
+  # The per-key mutex is evicted immediately after caching so KEY_MUTEXES stays
+  # bounded to keys currently being fetched for the first time (zero under
+  # steady state). See evict_key_mutex for the thread-safety analysis.
+  REGISTRATION_SCRIPT_CACHE_TTL = 5 * 60 # seconds
+
+  class << self
+    KEY_MUTEXES_LOCK = Mutex.new
+    KEY_MUTEXES      = {}
+    SCRIPT_CACHE     = Concurrent::Map.new
+
+    def registration_script_cache
+      SCRIPT_CACHE
+    end
+
+    def key_mutex(cache_key)
+      KEY_MUTEXES_LOCK.synchronize { KEY_MUTEXES[cache_key] ||= Mutex.new }
+    end
+
+    def evict_key_mutex(cache_key)
+      KEY_MUTEXES_LOCK.synchronize { KEY_MUTEXES.delete(cache_key) }
+    end
+  end
+
   get '/' do
-    response = Proxy::Registration::ProxyRequest.new.global_register(request)
-    handle_response(response)
+    cache_key = Rack::Utils.build_query(
+      Rack::Utils.parse_nested_query(request.query_string).sort_by { |k, _| k }
+    )
+
+    # Fast path — no lock needed, served directly from cache
+    cached = read_registration_cache(cache_key)
+    return cached if cached
+
+    # Slow path — serialize only threads competing for the same key.
+    # Threads for different keys (different activation keys / OS versions)
+    # fetch from Foreman in parallel without blocking each other.
+    fetch_and_cache_script(cache_key)
   rescue StandardError => e
     logger.exception "Error when rendering Global Registration Template", e
     render_error(default_error_msg)
@@ -19,11 +65,35 @@ class Proxy::Registration::Api < ::Sinatra::Base
 
   private
 
+  def fetch_and_cache_script(cache_key)
+    self.class.key_mutex(cache_key).synchronize do
+      # Re-check under lock: another thread with the same key may have
+      # already populated the cache while we were waiting.
+      cached = read_registration_cache(cache_key)
+      return cached if cached
+
+      response = Proxy::Registration::ProxyRequest.new.global_register(request)
+
+      if response.code.start_with?('2')
+        self.class.registration_script_cache[cache_key] = { body: response.body, at: Time.now }
+        self.class.evict_key_mutex(cache_key)
+        return response.body
+      end
+
+      message = response["content-type"].include?('text/plain') ? response.body : default_error_msg
+      render_error(message, code: response.code)
+    end
+  end
+
+  def read_registration_cache(cache_key)
+    entry = self.class.registration_script_cache[cache_key]
+    entry[:body] if entry && (Time.now - entry[:at]) < REGISTRATION_SCRIPT_CACHE_TTL
+  end
+
   def handle_response(response)
     if response.code.start_with? '2'
       response.body
     else
-      # Return error message only if it is not HTML.
       message = response["content-type"].include?('text/plain') ? response.body : default_error_msg
       render_error(message, code: response.code)
     end

test/registration/registration_api_test.rb

@@ -11,6 +11,9 @@ def app
   def setup
     @foreman_url = 'http://foreman.example.com'
     Proxy::SETTINGS.stubs(:foreman_url).returns(@foreman_url)
+    # Clear class-level state between tests to prevent cross-test contamination
+    Proxy::Registration::Api.registration_script_cache.clear
+    Proxy::Registration::Api::KEY_MUTEXES.clear
   end
 
   def test_global_register_template
@@ -102,6 +105,85 @@ def test_global_500
     assert_match("echo \"Internal Server Error\"\nexit 1\n", last_response.body)
   end
 
+  def test_global_register_caches_response
+    stub = stub_request(:get, "#{@foreman_url}/register").to_return(body: 'template')
+
+    2.times do
+      get '/'
+      assert last_response.ok?
+      assert_match('template', last_response.body)
+    end
+
+    assert_requested stub, times: 1
+  end
+
+  def test_global_register_cache_key_is_parameter_order_independent
+    stub = stub_request(:get, "#{@foreman_url}/register")
+             .with(query: { 'owner' => 'Default_Organization', 'activation_keys' => 'rhel9' })
+             .to_return(body: 'template')
+
+    get '/', { owner: 'Default_Organization', activation_keys: 'rhel9' }
+    assert last_response.ok?
+
+    # Different parameter order — must hit cache, not Foreman again
+    get '/', { activation_keys: 'rhel9', owner: 'Default_Organization' }
+    assert last_response.ok?
+
+    assert_requested stub, times: 1
+  end
+
+  def test_global_register_caches_per_key
+    stub_a = stub_request(:get, "#{@foreman_url}/register?key=a").to_return(body: 'template_a')
+    stub_b = stub_request(:get, "#{@foreman_url}/register?key=b").to_return(body: 'template_b')
+
+    get '/', { key: 'a' }
+    assert_match('template_a', last_response.body)
+    get '/', { key: 'b' }
+    assert_match('template_b', last_response.body)
+    # second requests — must be served from cache
+    get '/', { key: 'a' }
+    assert_match('template_a', last_response.body)
+    get '/', { key: 'b' }
+    assert_match('template_b', last_response.body)
+
+    assert_requested stub_a, times: 1
+    assert_requested stub_b, times: 1
+  end
+
+  def test_global_register_evicts_mutex_after_caching
+    stub_request(:get, "#{@foreman_url}/register").to_return(body: 'template')
+
+    get '/'
+    assert last_response.ok?
+    assert_empty Proxy::Registration::Api::KEY_MUTEXES
+  end
+
+  def test_global_register_cache_entry_expires_after_ttl
+    Proxy::Registration::Api.registration_script_cache[''] = {
+      body: 'stale-template',
+      at: Time.now - Proxy::Registration::Api::REGISTRATION_SCRIPT_CACHE_TTL - 1
+    }
+    stub = stub_request(:get, "#{@foreman_url}/register").to_return(body: 'fresh-template')
+
+    get '/'
+    assert last_response.ok?
+    assert_match('fresh-template', last_response.body)
+    assert_requested stub, times: 1
+  end
+
+  def test_global_register_does_not_cache_errors
+    stub = stub_request(:get, "#{@foreman_url}/register").to_return(
+      body: 'error', status: 500, headers: { "Content-Type" => 'text/plain' }
+    )
+
+    2.times do
+      get '/'
+      assert last_response.server_error?
+    end
+
+    assert_requested stub, times: 2
+  end
+
   def test_host_500
     Rack::NullLogger.any_instance.stubs(:exception)
     stub_request(:post, "#{@foreman_url}/register").to_timeout