From b4cc6ed11cc62dec5ca55c076f62791e90799578 Mon Sep 17 00:00:00 2001 From: femalves Date: Wed, 9 Sep 2026 14:58:25 -0400 Subject: [PATCH] scope cache keys per deployment --- scan_explorer_service/tests/test_cache.py | 118 ++++++++++++++++++++++ scan_explorer_service/tests/test_perf.py | 14 +-- scan_explorer_service/utils/cache.py | 63 ++++++++---- 3 files changed, 167 insertions(+), 28 deletions(-) diff --git a/scan_explorer_service/tests/test_cache.py b/scan_explorer_service/tests/test_cache.py index fa76d95..7af4a19 100644 --- a/scan_explorer_service/tests/test_cache.py +++ b/scan_explorer_service/tests/test_cache.py @@ -414,5 +414,123 @@ def test_ocr_served_from_cache(self, mock_cache_get): self.assertIn('text/plain', r.content_type) + +class TestVariantIsolation(TestCaseDatabase): + """Two deployments serving different hostnames must not share cached documents.""" + + def create_app(self): + from scan_explorer_service.app import create_app + return create_app(**{ + 'SQLALCHEMY_DATABASE_URI': self.postgresql_url, + 'TESTING': True, + 'PROXY_SERVER': 'https://ui.adsabs.harvard.edu:443', + 'PROXY_PREFIX': '/v1/scan', + }) + + def setUp(self): + super().setUp() + cache_mod._redis_client = None + + def tearDown(self): + cache_mod._redis_client = None + super().tearDown() + + @patch('scan_explorer_service.utils.cache.redis.from_url') + def test_variants_do_not_share_cached_manifests(self, mock_from_url): + store = {} + client = MagicMock() + client.ping.return_value = True + client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v) + client.get.side_effect = store.get + mock_from_url.return_value = client + + bibcode = '1993ASPC...52..132K' + ads_manifest = '{"@id":"https://ui.adsabs.harvard.edu:443/v1/scan/..."}' + + cache_mod.cache_set_manifest(bibcode, ads_manifest) + self.assertEqual(cache_mod.cache_get_manifest(bibcode), ads_manifest) + + self.app.config['PROXY_SERVER'] = 'https://scixplorer.org:443' + self.app.config['PROXY_PREFIX'] = '/v1/scix-scan' + + self.assertIsNone( + cache_mod.cache_get_manifest(bibcode), + 'the SciX deployment must not read the manifest cached by the ADS deployment') + + scix_manifest = '{"@id":"https://scixplorer.org:443/v1/scix-scan/..."}' + cache_mod.cache_set_manifest(bibcode, scix_manifest) + self.assertEqual(cache_mod.cache_get_manifest(bibcode), scix_manifest) + + self.app.config['PROXY_SERVER'] = 'https://ui.adsabs.harvard.edu:443' + self.app.config['PROXY_PREFIX'] = '/v1/scan' + self.assertEqual(cache_mod.cache_get_manifest(bibcode), ads_manifest) + + @patch('scan_explorer_service.utils.cache.redis.from_url') + def test_variants_do_not_share_cached_searches(self, mock_from_url): + store = {} + client = MagicMock() + client.ping.return_value = True + client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v) + client.get.side_effect = store.get + mock_from_url.return_value = client + + cache_key = 'ocr:1993ASPC...52..132K:gas' + ads_annotations = '{"resources":[{"on":"https://ui.adsabs.harvard.edu:443/v1/scan/canvas/x"}]}' + + cache_mod.cache_set_search(cache_key, ads_annotations) + self.assertEqual(cache_mod.cache_get_search(cache_key), ads_annotations) + + self.app.config['PROXY_SERVER'] = 'https://scixplorer.org:443' + self.app.config['PROXY_PREFIX'] = '/v1/scix-scan' + + self.assertIsNone( + cache_mod.cache_get_search(cache_key), + 'the SciX deployment must not read content-search annotations cached by ADS') + + @patch('scan_explorer_service.utils.cache.redis.from_url') + def test_delete_removes_the_id_from_every_recorded_scope(self, mock_from_url): + store = {} + scopes = set() + client = MagicMock() + client.ping.return_value = True + client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v) + client.get.side_effect = store.get + client.sadd.side_effect = lambda k, v: scopes.add(v) + client.smembers.side_effect = lambda k: set(scopes) + client.delete.side_effect = lambda k: store.pop(k, None) + mock_from_url.return_value = client + + collection_id = 'ApJ0099' + cache_mod.cache_set_manifest(collection_id, '{"ads":1}') + + self.app.config['PROXY_SERVER'] = 'https://scixplorer.org:443' + self.app.config['PROXY_PREFIX'] = '/v1/scix-scan' + cache_mod.cache_set_manifest(collection_id, '{"scix":1}') + + self.assertEqual(len(store), 2) + cache_mod.cache_delete_manifest(collection_id) + self.assertEqual(store, {}, 'the collection PUT must clear both deployment scopes') + + @patch('scan_explorer_service.utils.cache.redis.from_url') + def test_delete_leaves_other_ids_alone(self, mock_from_url): + store = {} + scopes = set() + client = MagicMock() + client.ping.return_value = True + client.setex.side_effect = lambda k, ttl, v: store.__setitem__(k, v) + client.sadd.side_effect = lambda k, v: scopes.add(v) + client.smembers.side_effect = lambda k: set(scopes) + client.delete.side_effect = lambda k: store.pop(k, None) + mock_from_url.return_value = client + + cache_mod.cache_set_manifest('ApJ0099', '{"a":1}') + cache_mod.cache_set_manifest('ApJ00990', '{"b":1}') + cache_mod.cache_set_manifest('*', '{"c":1}') + + cache_mod.cache_delete_manifest('ApJ0099') + + remaining = sorted(k.rsplit(':', 1)[-1] for k in store) + self.assertEqual(remaining, ['*', 'ApJ00990']) + if __name__ == '__main__': unittest.main() diff --git a/scan_explorer_service/tests/test_perf.py b/scan_explorer_service/tests/test_perf.py index 1bd2762..5af44b6 100644 --- a/scan_explorer_service/tests/test_perf.py +++ b/scan_explorer_service/tests/test_perf.py @@ -5,7 +5,7 @@ from unittest.mock import patch, MagicMock from scan_explorer_service.tests.base import TestCaseDatabase from scan_explorer_service.models import Article, Base, Collection, Page -from scan_explorer_service.utils.cache import cache_set_manifest, MANIFEST_CACHE_PREFIX +from scan_explorer_service.utils.cache import cache_set_manifest, MANIFEST_CACHE_PREFIX, _variant_scope from scan_explorer_service.views.image_proxy import fetch_images import scan_explorer_service.utils.cache as cache_mod @@ -95,7 +95,7 @@ def mock_delete(key): def test_cache_hit_returns_cached_json(self): """Verifies that a cached manifest is returned directly without regeneration.""" mock_r, store = self._mock_redis() - store[MANIFEST_CACHE_PREFIX + self.article.id] = ('{"@type":"sc:Manifest","cached":true}', time.monotonic() + 3600) + store[MANIFEST_CACHE_PREFIX + _variant_scope() + self.article.id] = ('{"@type":"sc:Manifest","cached":true}', time.monotonic() + 3600) url = url_for("manifest.get_manifest", id=self.article.id) r = self.client.get(url) @@ -106,7 +106,7 @@ def test_cache_hit_returns_cached_json(self): def test_cache_hit_returns_correct_content_type(self): """Verifies that cached manifest responses have application/json content type.""" mock_r, store = self._mock_redis() - store[MANIFEST_CACHE_PREFIX + self.collection.id] = ('{"@type":"sc:Manifest"}', time.monotonic() + 3600) + store[MANIFEST_CACHE_PREFIX + _variant_scope() + self.collection.id] = ('{"@type":"sc:Manifest"}', time.monotonic() + 3600) url = url_for("manifest.get_manifest", id=self.collection.id) r = self.client.get(url) @@ -128,12 +128,14 @@ def tracking_setex(key, ttl, val): cache_set_manifest(self.article.id, '{"@type":"sc:Manifest"}') self.assertEqual(len(setex_calls), 1) - self.assertEqual(setex_calls[0], MANIFEST_CACHE_PREFIX + self.article.id) + self.assertEqual( + setex_calls[0], + 'scan:manifest:http://localhost:8184/v1/scan:' + self.article.id) def test_cached_manifest_skips_manifest_factory(self): """Verifies that manifest_factory is not called when the manifest is cached.""" mock_r, store = self._mock_redis() - store[MANIFEST_CACHE_PREFIX + self.article.id] = ('{"@type":"sc:Manifest"}', time.monotonic() + 3600) + store[MANIFEST_CACHE_PREFIX + _variant_scope() + self.article.id] = ('{"@type":"sc:Manifest"}', time.monotonic() + 3600) with patch('scan_explorer_service.views.manifest.manifest_factory') as mock_factory: url = url_for("manifest.get_manifest", id=self.article.id) @@ -148,7 +150,7 @@ def test_404_not_cached(self): url = url_for("manifest.get_manifest", id='nonexistent') r = self.client.get(url) self.assertStatus(r, 404) - self.assertNotIn(MANIFEST_CACHE_PREFIX + 'nonexistent', store) + self.assertNotIn(MANIFEST_CACHE_PREFIX + _variant_scope() + 'nonexistent', store) def test_redis_unavailable_falls_through(self): """Verifies that the endpoint still works when Redis is unavailable.""" diff --git a/scan_explorer_service/utils/cache.py b/scan_explorer_service/utils/cache.py index 3f7c034..07b898c 100644 --- a/scan_explorer_service/utils/cache.py +++ b/scan_explorer_service/utils/cache.py @@ -3,6 +3,7 @@ import threading import json as json_lib from flask import current_app +from scan_explorer_service.utils.utils import proxy_url logger = logging.getLogger(__name__) @@ -10,6 +11,7 @@ MANIFEST_CACHE_PREFIX = 'scan:manifest:' SEARCH_CACHE_TTL = 60 SEARCH_CACHE_PREFIX = 'scan:search:' +MANIFEST_SCOPE_SET = 'scan:manifest:scopes' _redis_client = None _redis_lock = threading.Lock() @@ -40,13 +42,23 @@ def _reset_redis(): _redis_client = None +def _variant_scope(): + """Return the deployment scope for cache keys. + + Cached documents embed absolute URLs built from PROXY_SERVER and PROXY_PREFIX, + so two deployments serving different hostnames must not share cache entries. + """ + server, prefix = proxy_url() + return f'{server}/{prefix}:' + + def _redis_get(prefix, key): - """Fetch a cached value by prefix + key, returning None on miss or failure.""" + """Fetch a cached value by prefix + scope + key, returning None on miss or failure.""" r = _get_redis() if r is None: return None try: - return r.get(prefix + key) + return r.get(prefix + _variant_scope() + key) except redis.ConnectionError: _reset_redis() return None @@ -54,45 +66,52 @@ def _redis_get(prefix, key): return None -def _redis_set(prefix, key, value, ttl): - """Store a value in Redis with the given prefix, key, and TTL.""" +def _redis_set(prefix, key, value, ttl, scope_set=None): + """Store a value under prefix + scope + key, recording the scope when scope_set is given.""" r = _get_redis() if r is None: return try: - r.setex(prefix + key, ttl, value) + scope = _variant_scope() + r.setex(prefix + scope + key, ttl, value) + if scope_set: + r.sadd(scope_set, scope) except redis.ConnectionError: _reset_redis() except Exception: logger.debug("Failed to write cache for key %s%s", prefix, key, exc_info=True) -def _redis_delete(prefix, key): - """Delete a cached entry by prefix + key.""" - r = _get_redis() - if r is None: - return - try: - r.delete(prefix + key) - except redis.ConnectionError: - _reset_redis() - except Exception: - logger.debug("Failed to delete cache for key %s%s", prefix, key, exc_info=True) - - def cache_get_manifest(key): """Fetch a cached manifest JSON string.""" return _redis_get(MANIFEST_CACHE_PREFIX, key) def cache_set_manifest(key, json_str): - """Cache a manifest JSON string with 1-hour TTL.""" - _redis_set(MANIFEST_CACHE_PREFIX, key, json_str, MANIFEST_CACHE_TTL) + """Cache a manifest JSON string with 24-hour TTL.""" + _redis_set(MANIFEST_CACHE_PREFIX, key, json_str, MANIFEST_CACHE_TTL, MANIFEST_SCOPE_SET) def cache_delete_manifest(key): - """Invalidate a cached manifest. Called when a collection is updated via PUT.""" - _redis_delete(MANIFEST_CACHE_PREFIX, key) + """Invalidate one manifest id in every deployment scope that has cached it. + + A collection PUT reaches only one deployment, but the update applies to all of + them, so this id is removed under each scope recorded in MANIFEST_SCOPE_SET. + Manifests of the articles inside the collection are not touched; they expire + on their own TTL. + """ + r = _get_redis() + if r is None: + return + try: + scopes = set(r.smembers(MANIFEST_SCOPE_SET) or ()) + scopes.add(_variant_scope()) + for scope in scopes: + r.delete(MANIFEST_CACHE_PREFIX + scope + key) + except redis.ConnectionError: + _reset_redis() + except Exception: + logger.warning("Failed to delete cached manifest for key %s", key, exc_info=True) def cache_get_search(key):