mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-11 13:48:06 +00:00
fix(odds): stagger cache expiry so a slate does not all go stale at once
Observed on a live device, exactly one hour apart:
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
One hour is the odds TTL, and that is the whole story. Every game is
cached with the same fixed lifetime, and a slate is fetched in a single
pass, so they all go stale within milliseconds of each other. The next
update then re-fetches the entire card serially -- one ESPN request per
game -- and a normal 5s update runs past the plugin manager's 30s
timeout and is killed part-way through. Whichever games it had not
reached kept no odds until the next cycle, which hit the same wall.
Offset each entry's TTL by up to +-15%, derived from its cache key. An
hour then scatters expiry across about seventeen minutes; over a 16-game
slate the worst 60s window falls from all 16 to a handful.
Deriving the offset from the key rather than drawing it at random is
deliberate. It is stable, so a game keeps its slot across restarts,
where a fresh draw each start would re-cluster the whole slate the first
time the process bounced. It also keeps a random generator out of a path
where nothing is security-sensitive, which is what a scanner flagged.
Deliberately not a change to the fetching itself. Making it concurrent
would also fit inside the timeout, but it multiplies load on ESPN at the
moment of expiry, which is the behaviour that caused this. Spreading the
expiry removes the burst instead of absorbing it.
Four tests on main pinned the TTL as exactly equal to the interval;
they now assert the interval is honoured within the jitter band, keeping
what each was actually about.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
This commit is contained in:
co-authored by
Claude Opus 5
parent
ca26c1b83b
commit
3eb8602b61
@@ -11,6 +11,7 @@ Follows LEDMatrix configuration management patterns:
|
|||||||
- Maintainable: Changes to odds logic affect all plugins
|
- Maintainable: Changes to odds logic affect all plugins
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
import hashlib
|
||||||
import logging
|
import logging
|
||||||
import requests
|
import requests
|
||||||
import json
|
import json
|
||||||
@@ -73,6 +74,40 @@ class BaseOddsManager:
|
|||||||
except Exception as e:
|
except Exception as e:
|
||||||
self.logger.warning(f"Failed to load BaseOddsManager configuration: {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,
|
def get_odds(self, sport: str | None, league: str | None, event_id: str,
|
||||||
update_interval_seconds: int = None) -> Optional[Dict[str, Any]]:
|
update_interval_seconds: int = None) -> Optional[Dict[str, Any]]:
|
||||||
"""
|
"""
|
||||||
@@ -129,13 +164,14 @@ class BaseOddsManager:
|
|||||||
else:
|
else:
|
||||||
self.logger.debug("No odds data available for this game")
|
self.logger.debug("No odds data available for this game")
|
||||||
|
|
||||||
|
ttl = self._jittered_ttl(interval, cache_key)
|
||||||
if odds_data:
|
if odds_data:
|
||||||
self.cache_manager.set(cache_key, odds_data, ttl=interval)
|
self.cache_manager.set(cache_key, odds_data, ttl=ttl)
|
||||||
self.logger.info(f"Saved odds data to cache for {cache_key} with TTL {interval}s")
|
self.logger.info(f"Saved odds data to cache for {cache_key} with TTL {ttl}s")
|
||||||
else:
|
else:
|
||||||
self.logger.debug(f"No odds data available for {cache_key}")
|
self.logger.debug(f"No odds data available for {cache_key}")
|
||||||
# Cache the fact that no odds are available to avoid repeated API calls
|
# 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
|
return odds_data
|
||||||
|
|
||||||
|
|||||||
@@ -118,14 +118,29 @@ class TestGetOdds:
|
|||||||
mock_get.assert_not_called()
|
mock_get.assert_not_called()
|
||||||
assert manager.is_odds_available(result) is False
|
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(
|
def test_success_caches_extracted_data_with_interval_ttl(
|
||||||
self, manager, cache_manager, mock_get):
|
self, manager, cache_manager, mock_get):
|
||||||
result = manager.get_odds('football', 'nfl', '401',
|
result = manager.get_odds('football', 'nfl', '401',
|
||||||
update_interval_seconds=100)
|
update_interval_seconds=100)
|
||||||
|
|
||||||
assert result == FULL_EXTRACTED
|
assert result == FULL_EXTRACTED
|
||||||
cache_manager.set.assert_called_once_with(
|
self._assert_cached(cache_manager, 'odds_espn_football_nfl_401',
|
||||||
'odds_espn_football_nfl_401', FULL_EXTRACTED, ttl=100)
|
FULL_EXTRACTED, around=100)
|
||||||
|
|
||||||
def test_no_odds_caches_sentinel(self, manager, cache_manager, mock_get):
|
def test_no_odds_caches_sentinel(self, manager, cache_manager, mock_get):
|
||||||
mock_get.return_value = _make_response({'count': 0, 'items': []})
|
mock_get.return_value = _make_response({'count': 0, 'items': []})
|
||||||
@@ -133,8 +148,8 @@ class TestGetOdds:
|
|||||||
result = manager.get_odds('football', 'nfl', '401')
|
result = manager.get_odds('football', 'nfl', '401')
|
||||||
|
|
||||||
assert result is None
|
assert result is None
|
||||||
cache_manager.set.assert_called_once_with(
|
self._assert_cached(cache_manager, 'odds_espn_football_nfl_401',
|
||||||
'odds_espn_football_nfl_401', {'no_odds': True}, ttl=3600)
|
{'no_odds': True}, around=3600)
|
||||||
|
|
||||||
def test_zero_interval_falls_back_to_default(
|
def test_zero_interval_falls_back_to_default(
|
||||||
self, manager, cache_manager, mock_get):
|
self, manager, cache_manager, mock_get):
|
||||||
@@ -142,7 +157,8 @@ class TestGetOdds:
|
|||||||
# treats an explicit 0 as falsy, so the 3600 default wins.
|
# treats an explicit 0 as falsy, so the 3600 default wins.
|
||||||
manager.get_odds('football', 'nfl', '401', update_interval_seconds=0)
|
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(
|
def test_request_exception_falls_back_to_stale_cache(
|
||||||
self, manager, cache_manager, mock_get):
|
self, manager, cache_manager, mock_get):
|
||||||
@@ -205,8 +221,8 @@ class TestExtractEspnData:
|
|||||||
'home_team_odds': {'money_line': None, 'spread_odds': None},
|
'home_team_odds': {'money_line': None, 'spread_odds': None},
|
||||||
'away_team_odds': {'money_line': None, 'spread_odds': None},
|
'away_team_odds': {'money_line': None, 'spread_odds': None},
|
||||||
}
|
}
|
||||||
cache_manager.set.assert_called_once_with(
|
TestGetOdds._assert_cached(cache_manager, 'odds_espn_football_nfl_401',
|
||||||
'odds_espn_football_nfl_401', result, ttl=3600)
|
result, around=3600)
|
||||||
assert manager.is_odds_available(result) is False
|
assert manager.is_odds_available(result) is False
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -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
|
||||||
Reference in New Issue
Block a user