Fixes #22158: Cache empty config revision state to avoid per-request queries (#22342)

When no ConfigRevision exists, the empty state was never cached, so every
request re-queried core_configrevision. Distinguish a genuine cache miss from
a cached-empty state via a sentinel, seed the empty state on first load, and
only consult the database on a true miss.

Treat the cache as warm only when both 'config' and 'config_version' are
present. A missing 'config_version' (evicted or never written) now re-queries
the database instead of leaving Config.version as None when a ConfigRevision
exists. The no-revision branch writes both keys, so the intentional empty
state remains a cache hit.

The config tests shared a single Redis instance (keyed only by a static
prefix) across parallel test workers, so a no-revision test in one worker
could seed empty config/config_version keys that another worker's test then
read, causing intermittent failures. Use a per-process LocMemCache so the
shared cache keys cannot be contaminated across workers.

netbox/netbox/config/__init__.py

@@ -20,6 +20,10 @@ _thread_locals = threading.local()
 
 logger = logging.getLogger('netbox.config')
 
+# Sentinel used to distinguish a cache miss from a cached "empty" config (an empty dict is a
+# legitimate cached value when no ConfigRevision exists).
+_MISSING = object()
+
 
 def get_config():
     """
@@ -47,7 +51,9 @@ class Config:
     """
     def __init__(self):
         self._populate_from_cache()
-        if not self.config or not self.version:
+        # Only consult the database when the cache has genuinely never been populated. A cached
+        # empty config (no ConfigRevision) is authoritative and must not trigger a re-query.
+        if self._cache_miss:
             self._populate_from_db()
         self.defaults = {param.name: param.default for param in PARAMS}
 
@@ -69,8 +75,19 @@ class Config:
 
     def _populate_from_cache(self):
         """Populate config data from Redis cache"""
-        self.config = cache.get('config') or {}
-        self.version = cache.get('config_version')
+        cached_config = cache.get('config', _MISSING)
+        cached_version = cache.get('config_version', _MISSING)
+
+        # Treat the cache as warm only when both keys are present. A missing 'config_version'
+        # (e.g. evicted or never written) must re-query the database, even if 'config' is cached.
+        # The no-revision branch writes both keys (config={}, config_version=None), so the
+        # intentional empty state is still a cache hit.
+        self._cache_miss = cached_config is _MISSING or cached_version is _MISSING
+
+        # A cached value of None (ConfigRevision.data is nullable) or {} is a legitimate empty
+        # config and must not crash attribute access; normalize it to an empty dict.
+        self.config = {} if cached_config is _MISSING else (cached_config or {})
+        self.version = None if cached_version is _MISSING else cached_version
         if self.config:
             logger.debug("Loaded configuration data from cache")
 
@@ -86,10 +103,17 @@ class Config:
             revision = ConfigRevision.objects.order_by('-created').first()
             if revision is None:
                 logger.debug("No configuration found in database; proceeding with default values")
+                # Cache the empty state so subsequent requests are served from the cache rather than
+                # re-querying the database on every request (#22158). Creating the first
+                # ConfigRevision overwrites this via the post_save handler.
+                cache.set('config', {}, None)
+                cache.set('config_version', None, None)
+                self._populate_from_cache()
                 return
             logger.debug(f"No active configuration revision found; falling back to most recent (#{revision.pk})")
         except DatabaseError:
-            # The database may not be available yet (e.g. when running a management command)
+            # The database may not be available yet (e.g. when running a management command). Do NOT
+            # cache anything here, so the next instantiation re-queries once the database is reachable.
             logger.warning("Skipping config initialization (database unavailable)")
             return
 

netbox/netbox/tests/test_config.py

@@ -1,22 +1,31 @@
-from django.conf import settings
 from django.core.cache import cache
+from django.db import connection
 from django.test import TestCase, override_settings
+from django.test.utils import CaptureQueriesContext
 
 from core.models import ConfigRevision
 from netbox.config import clear_config, get_config
 
-# Prefix cache keys to avoid interfering with the local environment
-CACHES = settings.CACHES
-CACHES['default'].update({'KEY_PREFIX': 'TEST-'})
 
+def _configrevision_query_count(queries):
+    """Count captured queries that touch the core_configrevision table."""
+    return len([q for q in queries if 'core_configrevision' in q['sql']])
 
-@override_settings(CACHES=CACHES)
+
+# Use a per-process in-memory cache so the shared 'config'/'config_version' keys can't be
+# contaminated by other tests running in parallel against the same Redis instance.
+@override_settings(CACHES={
+    'default': {
+        'BACKEND': 'django.core.cache.backends.locmem.LocMemCache',
+        'LOCATION': 'netbox-config-tests',
+    },
+})
 class ConfigTestCase(TestCase):
 
     def setUp(self):
         super().setUp()
         # Register cleanup so it runs after each test, even on assertion failure.
-        # clear_config drops the thread-local Config; cache.clear wipes the Redis
+        # clear_config drops the thread-local Config; cache.clear wipes the cache
         # entries that configrevision.activate writes.
         self.addCleanup(cache.clear)
         self.addCleanup(clear_config)
@@ -30,6 +39,68 @@ class ConfigTestCase(TestCase):
         self.assertEqual(config.config, {})
         self.assertEqual(config.version, None)
 
+    def test_empty_config_not_requeried(self):
+        # With no ConfigRevision present, the empty state should be cached after the first load so
+        # that subsequent requests are served from the cache rather than re-querying the database
+        # on every request (#22158).
+        with CaptureQueriesContext(connection) as ctx:
+            first = get_config()
+        self.assertGreaterEqual(_configrevision_query_count(ctx.captured_queries), 1)
+        self.assertEqual(first.config, {})
+        self.assertEqual(first.version, None)
+
+        # Simulate the request boundary, where the middleware drops the thread-local config.
+        clear_config()
+
+        with CaptureQueriesContext(connection) as ctx:
+            second = get_config()
+        self.assertEqual(_configrevision_query_count(ctx.captured_queries), 0)
+        self.assertEqual(second.config, {})
+        self.assertEqual(second.version, None)
+
+    def test_empty_then_create_first_revision(self):
+        # Prime the cache with the empty state.
+        get_config()
+
+        # Creating the first ConfigRevision fires the post_save handler, which activates it and
+        # overwrites the cached empty state.
+        CONFIG_DATA = {'BANNER_TOP': 'A'}
+        configrevision = ConfigRevision.objects.create(data=CONFIG_DATA)
+
+        clear_config()
+        config = get_config()
+        self.assertEqual(config.config, CONFIG_DATA)
+        self.assertEqual(config.version, configrevision.pk)
+
+    def test_config_init_from_cache_null_data(self):
+        # ConfigRevision.data is nullable; a revision with data=None caches None. Config access
+        # must normalize that to an empty dict rather than crash on attribute lookup.
+        configrevision = ConfigRevision.objects.create(data=None)
+        configrevision.activate()
+
+        clear_config()
+        config = get_config()
+        self.assertEqual(config.config, {})
+        self.assertEqual(config.version, configrevision.pk)
+        # Attribute access must fall back to defaults without raising.
+        self.assertEqual(config.BANNER_TOP, '')
+
+    def test_missing_version_key_requeries(self):
+        # If 'config_version' is missing (evicted/never written) while 'config' is cached, the
+        # cache must be treated as cold and re-populated from the database rather than leaving
+        # version=None when a ConfigRevision exists.
+        CONFIG_DATA = {'BANNER_TOP': 'A'}
+        configrevision = ConfigRevision.objects.create(data=CONFIG_DATA)
+        configrevision.activate()
+
+        # Drop only the version key, leaving 'config' populated.
+        cache.delete('config_version')
+        clear_config()
+
+        config = get_config()
+        self.assertEqual(config.config, CONFIG_DATA)
+        self.assertEqual(config.version, configrevision.pk)
+
     def test_config_init_from_db(self):
         CONFIG_DATA = {'BANNER_TOP': 'A'}