perf: add caching for frequently-accessed stable data (#91) #120

Open

Claude wants to merge 1 commits from ai/major-domo-v2-91 into main

pull from: ai/major-domo-v2-91

merge into: cal:main

cal:main

cal:fix/submit-modal-missing-logger

cal:ai/major-domo-v2-82

cal:fix/scorecard-submission-resilience

cal:fix/utc-timezone-scheduling

cal:fix/scorebug-bugs

Conversation 1 Commits 1 Files Changed 4 +349 -261

Claude commented 2026-03-21 12:36:02 +00:00

Collaborator

Copy Link

Closes #91

Changes

league_service.get_current_state() — 60s Redis cache

Added @cached_single_item(ttl=60) decorator. Current week changes at most once per week; 60 seconds eliminates most redundant calls during transaction validation workflows.

standings_service.get_league_standings() — 10-minute in-memory cache

StandingsService doesn't inherit BaseService so can't use the standard decorators. Added a module-level _standings_cache: Dict[int, Tuple[List[TeamStandings], float]] with _STANDINGS_CACHE_TTL = 600, keyed by season. Follows the same pattern as _user_team_cache in utils/autocomplete.py (PR #100). All downstream methods (get_team_standings, get_standings_by_division, get_playoff_picture) benefit automatically since they call get_league_standings.

player_service.get_free_agents() — 5-minute Redis cache

Added @cached_api_call(ttl=300) decorator. get_top_free_agents calls this method then sorts client-side, so caching the full free-agent list benefits any caller regardless of the limit parameter.

dice/rolls.py _get_channel_embed_color() — 5-minute in-memory cache

Added module-level _channel_color_cache: Dict[int, Tuple[int, float]] keyed by channel ID with _CHANNEL_COLOR_CACHE_TTL = 300. The resolved color (from channel name abbrev lookup or user team lookup) is cached per channel. This eliminates up to 2 API calls per dice roll for /ab, /d20, /fielding, /jump.

Files changed

• services/league_service.py
• services/standings_service.py
• services/player_service.py
• commands/dice/rolls.py

Test results

977 passed, 2 skipped (no regressions)

Other observations

• rolls.py and league_service.py were incidentally reformatted by ruff (quote style, trailing commas, import grouping). Cosmetic only.
• The standings_service.py cache does not support Redis (unlike services that extend BaseService). This is intentional — adding Redis support would require either inheriting BaseService or adding a CacheManager, which is out of scope. The in-memory cache degrades gracefully to a per-process cache (fine for this use case).

Closes #91 ## Changes ### `league_service.get_current_state()` — 60s Redis cache Added `@cached_single_item(ttl=60)` decorator. Current week changes at most once per week; 60 seconds eliminates most redundant calls during transaction validation workflows. ### `standings_service.get_league_standings()` — 10-minute in-memory cache `StandingsService` doesn't inherit `BaseService` so can't use the standard decorators. Added a module-level `_standings_cache: Dict[int, Tuple[List[TeamStandings], float]]` with `_STANDINGS_CACHE_TTL = 600`, keyed by season. Follows the same pattern as `_user_team_cache` in `utils/autocomplete.py` (PR #100). All downstream methods (`get_team_standings`, `get_standings_by_division`, `get_playoff_picture`) benefit automatically since they call `get_league_standings`. ### `player_service.get_free_agents()` — 5-minute Redis cache Added `@cached_api_call(ttl=300)` decorator. `get_top_free_agents` calls this method then sorts client-side, so caching the full free-agent list benefits any caller regardless of the `limit` parameter. ### `dice/rolls.py _get_channel_embed_color()` — 5-minute in-memory cache Added module-level `_channel_color_cache: Dict[int, Tuple[int, float]]` keyed by channel ID with `_CHANNEL_COLOR_CACHE_TTL = 300`. The resolved color (from channel name abbrev lookup or user team lookup) is cached per channel. This eliminates up to 2 API calls per dice roll for `/ab`, `/d20`, `/fielding`, `/jump`. ## Files changed - `services/league_service.py` - `services/standings_service.py` - `services/player_service.py` - `commands/dice/rolls.py` ## Test results 977 passed, 2 skipped (no regressions) ## Other observations - `rolls.py` and `league_service.py` were incidentally reformatted by ruff (quote style, trailing commas, import grouping). Cosmetic only. - The `standings_service.py` cache does not support Redis (unlike services that extend BaseService). This is intentional — adding Redis support would require either inheriting BaseService or adding a CacheManager, which is out of scope. The in-memory cache degrades gracefully to a per-process cache (fine for this use case).

Claude added 1 commit 2026-03-21 12:36:03 +00:00

perf: add caching for frequently-accessed stable data (#91) ... 327570e8cc

Closes #91

- league_service.get_current_state(): @cached_single_item(ttl=60) — 60s Redis cache
- standings_service.get_league_standings(): in-memory dict cache with 10-minute TTL keyed by season
- player_service.get_free_agents(): @cached_api_call(ttl=300) — 5-minute Redis cache
- dice/rolls.py _get_channel_embed_color(): in-memory dict cache keyed by channel_id with 5-minute TTL, matching the autocomplete.py pattern from PR #100

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude referenced this pull request 2026-03-21 12:36:12 +00:00

perf: add caching for frequently-accessed stable data #91

Claude added the

ai-reviewing

label 2026-03-21 12:45:49 +00:00

Claude reviewed 2026-03-21 12:48:23 +00:00

Claude left a comment

Author

Collaborator

Copy Link

AI Code Review

Files Reviewed

• commands/dice/rolls.py (modified)
• services/league_service.py (modified)
• services/standings_service.py (modified)
• services/player_service.py (modified)

Findings

Correctness

All four caching implementations are correct:

• league_service.get_current_state() + @cached_single_item(ttl=60): Correct decorator choice for Optional[T] return. The decorator gracefully falls back to no-cache when self.cache/self._generate_cache_key are absent (Redis unavailable). No params in the signature → single stable cache key, which is correct since there's only one current state at a time. update_current_state calls get_current_state() internally to fetch the ID — the ID is stable across week updates, so cache staleness here is harmless.

• standings_service in-memory cache: TTL comparison (time.time() - cached_at < _STANDINGS_CACHE_TTL) is correct. Pattern matches PR #100's _user_team_cache exactly. get_standings_by_division, get_team_standings, and get_playoff_picture all call get_league_standings → all three benefit automatically and correctly. Callers iterate the returned list read-only, so returning the cached list reference is safe (no mutation risk).

• player_service.get_free_agents() + @cached_api_call(ttl=300): Cache key includes season parameter, so different seasons get separate entries. get_top_free_agents creates a new sorted copy, so the cached list is not mutated.

• rolls.py _get_channel_embed_color() channel cache: Cache-check at top, fallthrough on miss/expiry, write-back only on the team-by-abbrev path. interaction.channel_id is an Optional[int] and the channel_id is not None guard is correct. Module-level dict keyed by int channel ID is safe.

No off-by-one errors, null issues, or type mismatches found.

Security

No issues found. No user input reaches cache keys directly — keys are Discord channel IDs (integers) or method/parameter hashes internal to BaseService. No injection vectors.

Style & Conventions

• All three Redis-backed caches gracefully degrade without Redis — consistent with project's existing opt-in caching philosophy documented in utils/CLAUDE.md.
• In-memory caches follow the established pattern from utils/autocomplete.py (PR #100): module-level Dict[key, Tuple[value, float]] + _TTL constant.
• import time and from typing import Dict, Tuple additions in rolls.py are correct and in the right position. from utils.decorators import cached_single_item / cached_api_call additions in service files are also correct.
• Ruff reformatting (quote style, trailing commas, line wrapping) is cosmetic and consistent with project linting.

Suggestions

standings_service.recalculate_standings() doesn't clear the cache — after an admin triggers a recalculation, get_league_standings() will serve stale data for up to 10 minutes. Not a blocker (recalculation is rare and 10 minutes is short), but consider adding _standings_cache.pop(season, None) at the end of recalculate_standings for correctness:

async def recalculate_standings(self, season: int) -> bool:
    ...
    logger.info(f"Recalculated standings for season {season}")
    _standings_cache.pop(season, None)  # invalidate cached standings
    return True

_get_channel_embed_color() fallback path is not cached — when the channel name doesn't match an abbreviation pattern, the get_user_major_league_team call is still made on every dice roll. The PR description claims "eliminates up to 2 API calls per dice roll" but this only applies to team-named channels. For general channels, the user-team lookup remains uncached. Not a bug — team_service.get_team_by_abbrev already has a @cached_single_item decorator, so the abbrev lookup path IS doubly cached. The user-team fallback is the remaining optimization gap. Worth a follow-up if dice roll performance is a concern.

cached_api_call empty-list behavior (pre-existing) — if result and hasattr(self, "_cache_items") means an empty free-agent list ([]) is never cached, so a league with no free agents will hit the API on every call. This is a pre-existing characteristic of the decorator, not introduced here.

Verdict: COMMENT

All four caching implementations are correct and safe. The patterns are consistent with existing in-memory and Redis caches in the codebase. The only noteworthy gap is recalculate_standings() not invalidating the new cache — a minor correctness issue that is worth a one-line fix but doesn't block merging. 977 tests passing with no regressions.

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `commands/dice/rolls.py` (modified) - `services/league_service.py` (modified) - `services/standings_service.py` (modified) - `services/player_service.py` (modified) ### Findings #### Correctness All four caching implementations are correct: - **`league_service.get_current_state()` + `@cached_single_item(ttl=60)`**: Correct decorator choice for `Optional[T]` return. The decorator gracefully falls back to no-cache when `self.cache`/`self._generate_cache_key` are absent (Redis unavailable). No params in the signature → single stable cache key, which is correct since there's only one current state at a time. `update_current_state` calls `get_current_state()` internally to fetch the ID — the ID is stable across week updates, so cache staleness here is harmless. - **`standings_service` in-memory cache**: TTL comparison (`time.time() - cached_at < _STANDINGS_CACHE_TTL`) is correct. Pattern matches PR #100's `_user_team_cache` exactly. `get_standings_by_division`, `get_team_standings`, and `get_playoff_picture` all call `get_league_standings` → all three benefit automatically and correctly. Callers iterate the returned list read-only, so returning the cached list reference is safe (no mutation risk). - **`player_service.get_free_agents()` + `@cached_api_call(ttl=300)`**: Cache key includes `season` parameter, so different seasons get separate entries. `get_top_free_agents` creates a new sorted copy, so the cached list is not mutated. - **`rolls.py _get_channel_embed_color()` channel cache**: Cache-check at top, fallthrough on miss/expiry, write-back only on the team-by-abbrev path. `interaction.channel_id` is an `Optional[int]` and the `channel_id is not None` guard is correct. Module-level dict keyed by `int` channel ID is safe. No off-by-one errors, null issues, or type mismatches found. #### Security No issues found. No user input reaches cache keys directly — keys are Discord channel IDs (integers) or method/parameter hashes internal to `BaseService`. No injection vectors. #### Style & Conventions - All three Redis-backed caches gracefully degrade without Redis — consistent with project's existing opt-in caching philosophy documented in `utils/CLAUDE.md`. - In-memory caches follow the established pattern from `utils/autocomplete.py` (PR #100): module-level `Dict[key, Tuple[value, float]]` + `_TTL` constant. - `import time` and `from typing import Dict, Tuple` additions in `rolls.py` are correct and in the right position. `from utils.decorators import cached_single_item` / `cached_api_call` additions in service files are also correct. - Ruff reformatting (quote style, trailing commas, line wrapping) is cosmetic and consistent with project linting. #### Suggestions **`standings_service.recalculate_standings()` doesn't clear the cache** — after an admin triggers a recalculation, `get_league_standings()` will serve stale data for up to 10 minutes. Not a blocker (recalculation is rare and 10 minutes is short), but consider adding `_standings_cache.pop(season, None)` at the end of `recalculate_standings` for correctness: ```python async def recalculate_standings(self, season: int) -> bool: ... logger.info(f"Recalculated standings for season {season}") _standings_cache.pop(season, None) # invalidate cached standings return True ``` **`_get_channel_embed_color()` fallback path is not cached** — when the channel name doesn't match an abbreviation pattern, the `get_user_major_league_team` call is still made on every dice roll. The PR description claims "eliminates up to 2 API calls per dice roll" but this only applies to team-named channels. For general channels, the user-team lookup remains uncached. Not a bug — `team_service.get_team_by_abbrev` already has a `@cached_single_item` decorator, so the abbrev lookup path IS doubly cached. The user-team fallback is the remaining optimization gap. Worth a follow-up if dice roll performance is a concern. **`cached_api_call` empty-list behavior (pre-existing)** — `if result and hasattr(self, "_cache_items")` means an empty free-agent list (`[]`) is never cached, so a league with no free agents will hit the API on every call. This is a pre-existing characteristic of the decorator, not introduced here. ### Verdict: COMMENT All four caching implementations are correct and safe. The patterns are consistent with existing in-memory and Redis caches in the codebase. The only noteworthy gap is `recalculate_standings()` not invalidating the new cache — a minor correctness issue that is worth a one-line fix but doesn't block merging. 977 tests passing with no regressions. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-21 12:48:46 +00:00

cal force-pushed ai/major-domo-v2-91 from 327570e8cc to 61f36353d8 2026-03-31 19:41:35 +00:00 Compare

cal commented 2026-03-31 19:55:17 +00:00

Owner

Copy Link

Please fix the remaining correctness issue from the review:

The only noteworthy gap is recalculate_standings() not invalidating the new cache — a minor correctness issue that is worth a one-line fix but doesn't block merging. 977 tests passing with no regressions.

Please fix the remaining correctness issue from the review: > The only noteworthy gap is recalculate_standings() not invalidating the new cache — a minor correctness issue that is worth a one-line fix but doesn't block merging. 977 tests passing with no regressions.

cal removed the

ai-reviewed

label 2026-03-31 19:55:22 +00:00

Claude added the

ai-reviewing

label 2026-03-31 20:15:47 +00:00

Claude removed the

ai-reviewing

label 2026-03-31 20:20:48 +00:00

Claude added the

ai-reviewing

label 2026-03-31 20:45:46 +00:00

Claude reviewed 2026-03-31 20:49:46 +00:00

Claude left a comment

Author

Collaborator

Copy Link

AI Code Review

Files Reviewed

• commands/dice/rolls.py (modified)
• services/league_service.py (modified)
• services/player_service.py (modified)
• services/standings_service.py (modified)

Findings

Correctness

Bug: _channel_color_cache uses wrong key for user-team fallback path

_get_channel_embed_color() has two color-resolution paths:

1. Channel name has a team prefix (e.g. nyy-dice) → color comes from the channel's team → keyed by channel_id ✅ correct
2. Channel has no team prefix → color comes from the rolling user's team → also keyed by channel_id ❌ wrong

In path 2, if User A (Yankees) rolls /ab in #general and is cached, User B (Red Sox) rolling in the same channel within 5 minutes gets Yankees blue instead of their own color. The cache key needs to account for the user's identity in the fallback path.

Minimal fix — only cache the channel-name path (where the color is truly per-channel), and leave the user-team fallback path uncached:

if channel_team is not None and channel_team.color is not None:
    color = int(channel_team.color, 16)
    if channel_id is not None:
        _channel_color_cache[channel_id] = (color, time.time())  # ✅ keep
    return color

team = await get_user_major_league_team(user_id=interaction.user.id)
if team is not None and team.color is not None:
    color = int(team.color, 16)
    # Don't cache this — it's per-user, not per-channel
    return color

Alternatively, composite key (channel_id, user_id) would also work but adds complexity.

Note: team_service.get_team_by_abbrev is itself Redis-cached, so the user-team path already benefits from caching at the service level without needing a second layer here.

update_current_state() doesn't invalidate get_current_state() cache (existing concern)

After update_current_state(week=5, freeze=True) executes successfully, subsequent callers see stale data for up to 60s. Low-severity given the infrequency of writes (weekly freeze task), but worth a one-liner @cache_invalidate after the patch call in a follow-up.

Security

No issues found.

Style & Conventions

All caching patterns match the established patterns from PR #100 (_user_team_cache in autocomplete.py). Decorator usage in league_service and player_service matches services/CLAUDE.md guidance. Module-level _standings_cache correctly parallels the StandingsService-doesn't-extend-BaseService constraint. Cosmetic reformatting (single→double quotes, trailing commas) is ruff-enforced and consistent.

Suggestions

• recalculate_standings() still doesn't clear _standings_cache. One-liner: _standings_cache.pop(season, None) before return True. Admin recalculations will otherwise be invisible for up to 10 minutes. (Noted in prior review — still worth a follow-up.)

• EmbedColors.PRIMARY fallback (no team found) is correctly not cached, but the user-team path is being cached — see correctness bug above.

Verdict: COMMENT

Implementation is correct for the three service caches (@cached_single_item on get_current_state, @cached_api_call on get_free_agents, in-memory _standings_cache). The _channel_color_cache has a real correctness bug where the user-team fallback path is keyed by channel_id, causing cross-user color contamination in non-team channels. This is cosmetic (wrong embed color, not wrong data) and only affects users rolling dice in general channels, but it should be fixed before or shortly after merge.

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `commands/dice/rolls.py` (modified) - `services/league_service.py` (modified) - `services/player_service.py` (modified) - `services/standings_service.py` (modified) ### Findings #### Correctness **Bug: `_channel_color_cache` uses wrong key for user-team fallback path** `_get_channel_embed_color()` has two color-resolution paths: 1. Channel name has a team prefix (e.g. `nyy-dice`) → color comes from the channel's team → keyed by `channel_id` ✅ correct 2. Channel has no team prefix → color comes from the *rolling user's* team → also keyed by `channel_id` ❌ wrong In path 2, if User A (Yankees) rolls `/ab` in `#general` and is cached, User B (Red Sox) rolling in the same channel within 5 minutes gets Yankees blue instead of their own color. The cache key needs to account for the user's identity in the fallback path. Minimal fix — only cache the channel-name path (where the color is truly per-channel), and leave the user-team fallback path uncached: ```python if channel_team is not None and channel_team.color is not None: color = int(channel_team.color, 16) if channel_id is not None: _channel_color_cache[channel_id] = (color, time.time()) # ✅ keep return color team = await get_user_major_league_team(user_id=interaction.user.id) if team is not None and team.color is not None: color = int(team.color, 16) # Don't cache this — it's per-user, not per-channel return color ``` Alternatively, composite key `(channel_id, user_id)` would also work but adds complexity. **Note**: `team_service.get_team_by_abbrev` is itself Redis-cached, so the user-team path already benefits from caching at the service level without needing a second layer here. **`update_current_state()` doesn't invalidate `get_current_state()` cache** (existing concern) After `update_current_state(week=5, freeze=True)` executes successfully, subsequent callers see stale data for up to 60s. Low-severity given the infrequency of writes (weekly freeze task), but worth a one-liner `@cache_invalidate` after the patch call in a follow-up. #### Security No issues found. #### Style & Conventions All caching patterns match the established patterns from PR #100 (`_user_team_cache` in `autocomplete.py`). Decorator usage in `league_service` and `player_service` matches `services/CLAUDE.md` guidance. Module-level `_standings_cache` correctly parallels the `StandingsService`-doesn't-extend-BaseService constraint. Cosmetic reformatting (single→double quotes, trailing commas) is ruff-enforced and consistent. #### Suggestions - `recalculate_standings()` still doesn't clear `_standings_cache`. One-liner: `_standings_cache.pop(season, None)` before `return True`. Admin recalculations will otherwise be invisible for up to 10 minutes. (Noted in prior review — still worth a follow-up.) - `EmbedColors.PRIMARY` fallback (no team found) is correctly not cached, but the user-team path *is* being cached — see correctness bug above. ### Verdict: COMMENT Implementation is correct for the three service caches (`@cached_single_item` on `get_current_state`, `@cached_api_call` on `get_free_agents`, in-memory `_standings_cache`). The `_channel_color_cache` has a real correctness bug where the user-team fallback path is keyed by `channel_id`, causing cross-user color contamination in non-team channels. This is cosmetic (wrong embed color, not wrong data) and only affects users rolling dice in general channels, but it should be fixed before or shortly after merge. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-31 20:50:08 +00:00

This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin ai/major-domo-v2-91:ai/major-domo-v2-91

git checkout ai/major-domo-v2-91

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

ai-changes-requested

  

ai-pr-opened

  

ai-reviewed

  

ai-reviewing

  

ai-working

  

in-next-release

Fix included in the next-release branch, pending merge to main

  

status/in-progress

Work is actively in progress

  

status/pr-open

A pull request has been opened for this issue

No Label

ai-changes-requested

ai-pr-opened

ai-reviewed

ai-reviewing

ai-working

in-next-release

status/in-progress

status/pr-open

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

2 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-v2#120

No description provided.