diff --git a/src/base_odds_manager.py b/src/base_odds_manager.py index f35c4d54..1a14f477 100644 --- a/src/base_odds_manager.py +++ b/src/base_odds_manager.py @@ -11,6 +11,7 @@ Follows LEDMatrix configuration management patterns: - Maintainable: Changes to odds logic affect all plugins """ +import hashlib import logging import requests import json @@ -73,6 +74,40 @@ class BaseOddsManager: except Exception as e: self.logger.warning(f"Failed to load BaseOddsManager configuration: {e}") + # Spread of the cache TTL, as a fraction. Every game in a slate is fetched + # in one pass, so a fixed TTL expires them all within milliseconds of each + # other and the next update re-fetches the whole slate serially. Measured on + # a live device that took football-scoreboard's update() past the plugin + # manager's 30s timeout, killing it once an hour, every hour, and leaving + # the games it had not reached without odds. + _TTL_JITTER = 0.15 + + def _jittered_ttl(self, interval: int, cache_key: str) -> int: + """ + A cache lifetime near ``interval``, offset by a per-key amount. + + Staggering expiry is what stops a slate of games all going stale on the + same tick. The spread is proportional, so a one-hour interval scatters + expiries across about eighteen minutes and only a game or two comes due + per update instead of the whole card. + + The offset is derived from the key rather than drawn at random, which + is both more useful and more honest. More useful because it is stable: + a game keeps its slot across restarts, where a fresh random draw each + time would re-cluster the whole slate the first time the process + bounced. More honest because nothing here is security-sensitive, and + reaching for a random generator invites exactly the question of whether + it should be a cryptographic one. + """ + if interval <= 0: + return interval + # blake2b for speed and a clean 8-byte digest; any stable hash would + # do. Not a security boundary -- this only decides who expires first. + digest = hashlib.blake2b(cache_key.encode('utf-8'), digest_size=8).digest() + fraction = int.from_bytes(digest, 'big') / float(1 << 64) # 0.0 .. 1.0 + offset = (fraction * 2.0 - 1.0) * self._TTL_JITTER # -j .. +j + return max(1, int(interval * (1.0 + offset))) + def get_odds(self, sport: str | None, league: str | None, event_id: str, update_interval_seconds: int = None) -> Optional[Dict[str, Any]]: """ @@ -129,13 +164,14 @@ class BaseOddsManager: else: self.logger.debug("No odds data available for this game") + ttl = self._jittered_ttl(interval, cache_key) if odds_data: - self.cache_manager.set(cache_key, odds_data, ttl=interval) - self.logger.info(f"Saved odds data to cache for {cache_key} with TTL {interval}s") + self.cache_manager.set(cache_key, odds_data, ttl=ttl) + self.logger.info(f"Saved odds data to cache for {cache_key} with TTL {ttl}s") else: self.logger.debug(f"No odds data available for {cache_key}") # Cache the fact that no odds are available to avoid repeated API calls - self.cache_manager.set(cache_key, {"no_odds": True}, ttl=interval) + self.cache_manager.set(cache_key, {"no_odds": True}, ttl=ttl) return odds_data diff --git a/test/test_base_odds_manager.py b/test/test_base_odds_manager.py index b08183ad..86d50f5e 100644 --- a/test/test_base_odds_manager.py +++ b/test/test_base_odds_manager.py @@ -118,14 +118,29 @@ class TestGetOdds: mock_get.assert_not_called() assert manager.is_odds_available(result) is False + @staticmethod + def _assert_cached(cache_manager, key, value, around): + """The cache write, with a TTL near `around` rather than equal to it. + + Odds TTLs are jittered so a slate fetched in one pass does not expire + on the same tick; asserting equality here would pin the stampede that + killed football-scoreboard's update() once an hour. The interval must + still be honoured, so the range is tight. + """ + cache_manager.set.assert_called_once() + args, kwargs = cache_manager.set.call_args + assert args[0] == key + assert args[1] == value + assert around * 0.85 <= kwargs['ttl'] <= around * 1.15, kwargs['ttl'] + def test_success_caches_extracted_data_with_interval_ttl( self, manager, cache_manager, mock_get): result = manager.get_odds('football', 'nfl', '401', update_interval_seconds=100) assert result == FULL_EXTRACTED - cache_manager.set.assert_called_once_with( - 'odds_espn_football_nfl_401', FULL_EXTRACTED, ttl=100) + self._assert_cached(cache_manager, 'odds_espn_football_nfl_401', + FULL_EXTRACTED, around=100) def test_no_odds_caches_sentinel(self, manager, cache_manager, mock_get): mock_get.return_value = _make_response({'count': 0, 'items': []}) @@ -133,8 +148,8 @@ class TestGetOdds: result = manager.get_odds('football', 'nfl', '401') assert result is None - cache_manager.set.assert_called_once_with( - 'odds_espn_football_nfl_401', {'no_odds': True}, ttl=3600) + self._assert_cached(cache_manager, 'odds_espn_football_nfl_401', + {'no_odds': True}, around=3600) def test_zero_interval_falls_back_to_default( self, manager, cache_manager, mock_get): @@ -142,7 +157,8 @@ class TestGetOdds: # treats an explicit 0 as falsy, so the 3600 default wins. manager.get_odds('football', 'nfl', '401', update_interval_seconds=0) - assert cache_manager.set.call_args.kwargs['ttl'] == 3600 + # Still the 3600 default, now jittered around it rather than exact. + assert 3600 * 0.85 <= cache_manager.set.call_args.kwargs['ttl'] <= 3600 * 1.15 def test_request_exception_falls_back_to_stale_cache( self, manager, cache_manager, mock_get): @@ -205,8 +221,8 @@ class TestExtractEspnData: 'home_team_odds': {'money_line': None, 'spread_odds': None}, 'away_team_odds': {'money_line': None, 'spread_odds': None}, } - cache_manager.set.assert_called_once_with( - 'odds_espn_football_nfl_401', result, ttl=3600) + TestGetOdds._assert_cached(cache_manager, 'odds_espn_football_nfl_401', + result, around=3600) assert manager.is_odds_available(result) is False diff --git a/test/test_odds_cache_stampede.py b/test/test_odds_cache_stampede.py new file mode 100644 index 00000000..832d885d --- /dev/null +++ b/test/test_odds_cache_stampede.py @@ -0,0 +1,116 @@ +"""Tests that odds cache entries do not all expire on the same tick. + +Regression under test: every game's odds were cached with the same fixed TTL, +and a slate is fetched in one pass, so they all went stale within milliseconds +of each other. The next update then re-fetched the entire card serially. On a +live device that pushed football-scoreboard's update() past the plugin +manager's 30s timeout: + + 00:43:43 ERROR plugin football-scoreboard operation timed out after 30.0s + 01:43:43 ERROR plugin football-scoreboard operation timed out after 30.0s + +Exactly one hour apart -- the odds TTL -- with a normal update taking ~5s. The +update was killed part-way, so whichever games it had not reached kept no odds +until the next cycle, which then hit the same wall. +""" + +from unittest.mock import Mock + +from src.base_odds_manager import BaseOddsManager + + +def _manager(): + return BaseOddsManager(cache_manager=Mock(), config_manager=None) + + +def _keys(n): + return ["odds_espn_football_nfl_4018730%02d" % i for i in range(n)] + + +class TestJitteredTtl: + def test_different_keys_get_different_ttls(self): + m = _manager() + ttls = {m._jittered_ttl(3600, k) for k in _keys(50)} + assert len(ttls) > 1, "a constant TTL is exactly the bug" + + def test_the_same_key_always_gets_the_same_ttl(self): + # Derived from the key, not drawn at random, so a game keeps its slot + # across restarts. A fresh draw each start would re-cluster the slate + # the first time the process bounced. + m = _manager() + first = m._jittered_ttl(3600, "odds_espn_football_nfl_401") + assert all(m._jittered_ttl(3600, "odds_espn_football_nfl_401") == first + for _ in range(20)) + assert _manager()._jittered_ttl(3600, "odds_espn_football_nfl_401") == first + + def test_ttl_stays_close_to_the_requested_interval(self): + # Odds must not go stale far early or linger far too long. + m = _manager() + for k in _keys(200): + ttl = m._jittered_ttl(3600, k) + assert 3600 * 0.85 <= ttl <= 3600 * 1.15, ttl + + def test_spread_is_wide_enough_to_matter(self): + # An hour interval should scatter expiries across many minutes, so only + # a game or two comes due per update rather than the whole card. + m = _manager() + ttls = [m._jittered_ttl(3600, k) for k in _keys(200)] + assert max(ttls) - min(ttls) > 600, ( + "spread of %ds is too narrow to break up the stampede" + % (max(ttls) - min(ttls))) + + def test_short_intervals_stay_positive(self): + m = _manager() + for interval in (1, 2, 5, 30): + for k in _keys(20): + assert m._jittered_ttl(interval, k) >= 1 + + def test_zero_and_negative_pass_through(self): + # 0 often means "no expiry" to a cache; do not turn that into 1. + m = _manager() + assert m._jittered_ttl(0, "k") == 0 + assert m._jittered_ttl(-1, "k") == -1 + + +class TestNoStampede: + def test_a_slate_fetched_together_does_not_expire_together(self): + """The behaviour that actually matters, modelled over a full slate.""" + m = _manager() + interval = 3600 + # 16 games, all fetched in the same pass at t=0. + expiries = sorted(m._jittered_ttl(interval, k) for k in _keys(16)) + + # How many come due within any 60s update window? + worst = max( + sum(1 for e in expiries if t <= e < t + 60) + for t in range(min(expiries), max(expiries) + 1, 10) + ) + assert worst < 16, "the whole slate still expires in one window" + assert worst <= 6, ( + "%d of 16 games still come due in a single 60s window" % worst) + + def test_ttl_reaches_the_cache_layer(self): + """The jitter is pointless if the value never gets used.""" + cache = Mock() + cache.get_with_auto_strategy.return_value = None + m = BaseOddsManager(cache_manager=cache, config_manager=None) + + # Force the no-odds path, which is the simplest write to observe. + m._extract_espn_data = Mock(return_value=None) + import src.base_odds_manager as mod + real_get = mod.requests.get + try: + resp = Mock() + resp.json.return_value = {} + resp.raise_for_status.return_value = None + mod.requests.get = Mock(return_value=resp) + m.get_odds("football", "nfl", "401873042") + finally: + mod.requests.get = real_get + + assert cache.set.called + ttl = cache.set.call_args.kwargs.get("ttl") + assert ttl is not None, "ttl was not passed to the cache" + # A single call may legitimately land on 3600, so the assertion is the + # range; that the value varies is covered above. + assert 3600 * 0.85 <= ttl <= 3600 * 1.15, ttl