# Code Review Report: group_horse_racing
**Date**: 2026-05-09  
**Scope**: 15 Python files, ~200KB  
**Files Modified**: 4

## Summary
Horse racing plugin with room management, betting system, race simulation, and settlement.  
Overall architecture is clean (command pattern + engine + store separation). Found 1 critical singleton bug and 1 enum bug.

## Issues Found & Fixed

### FIX 1 — CRITICAL: Dual RoomStore Instances (shared.py)
- **Problem**: `shared.py` created its own `RoomStore(Config())` at module level (L18), separate from the singleton in `room_store.py` (L253) managed by `__init__.py` lifecycle hooks. This meant the `startup`/`shutdown` hooks (init DB, cleanup old rooms) operated on a DIFFERENT instance than the commands module.
- **Impact**: Race data persistence could silently fail — rooms might not save to DB, old rooms not cleaned up.
- **Fix**: Changed `from ..room_store import RoomStore` + `room_store = RoomStore(config)` → `from ..room_store import room_store` (import the singleton).
- **Risk**: Low — straightforward import change.

### FIX 2 — BUG: Invalid HorseState.WAITING (race.py L146)
- **Problem**: After stopping a race, horses were set to `HorseState.WAITING` which doesn't exist in the enum (only READY/RACING/FINISHED).
- **Impact**: Would raise `AttributeError` at runtime if stop-race command was used.
- **Fix**: Changed to `HorseState.READY`.
- **Risk**: None — enum value now exists.

### FIX 3 — Silent Exceptions → Debug Logging (message_service.py)
- **Problem**: Two `except Exception: pass` blocks in `recall_previous_of_type` (L66) and `_schedule_recall` (L81).
- **Context**: Message deletion failures (network errors, already deleted).
- **Fix**: Added `logger.debug(..., exc_info=True)` for observability.
- **Risk**: None — logging only.

### FIX 4 — Silent Exceptions → Debug Logging (test_commands.py)
- **Problem**: Two `except Exception: pass` blocks in test cleanup code (L256, L237).
- **Fix**: Added `logger.debug(...)` for test debugging.

## Issues Reviewed & Accepted (No Fix Needed)
- **config.py:75** — Silent `except ValueError: pass` with fallback to `set()`. Already has warning at L70. Radius-0 operation.
- **race.py:77,127** — Admin check silent excepts. Default to non-admin on API failure. Radius-0 operation.
- **shared.py:31** — Name lookup fallback to user_id string. Radius-0 operation.
- **test_commands.py L266** — `RoomStore()` in `_InMemoryRoomStore` mock. Test-only, acceptable.
- **image_render.py** — PIL synchronous rendering. Pre-existing in qqpush plugin (fixed there). Not actionable here as it's the same shared code.

## Architecture Notes
- Good separation: RoomStore (persistence) → RaceEngine (logic) → MessageService (messaging) → Commands (handlers)
- Singleton pattern for RoomStore with lifecycle management via nonebot hooks
- Race simulation runs as asyncio task with tick-based updates
- Betting system with odds calculation is well-structured
- Test file (413 lines) provides good simulation coverage

## Verification
- ✅ 15/15 files syntax valid
- ✅ No `HorseState.WAITING` references remain
- ✅ `shared.py` imports singleton (no `RoomStore(config)` call)
- ✅ Debug logging present in message_service.py and test_commands.py