fix: prevent partial DB writes on scorecard submission failure #80

cal · 2026-03-11T01:55:36Z

cal commented

2026-03-11 01:55:36 +00:00

Summary

Read all spreadsheet data (plays, box score, pitching decisions) before any database writes, preventing partial commits when the spreadsheet has formula errors (e.g. #N/A)
Preserve detailed SheetsException messages through the error chain instead of wrapping them in generic "Unable to read pitching decisions"
Add dedicated SheetsException handler that shows users the specific cell and error type instead of "An unexpected error occurred"

Context

On 2026-03-10, a CLS vs MKE scorecard submission failed because the Pitcherstats tab had a #N/A formula error in team_id (Row 4). The old code had already committed 74 plays and updated the game score before attempting to read pitching decisions — leaving the DB in a partial state. The user saw only: "An unexpected error occurred. Please try again or contact an admin."

What changed

commands/league/submit_scorecard.py: Moved all three spreadsheet reads into a single Phase 6 before any DB writes (Phase 7-9). Added except SheetsException handler with user-friendly detailed message.
services/sheets_service.py: Added except SheetsException: raise in read_pitching_decisions and read_box_score so detailed error messages aren't swallowed by the generic wrapper.

Test plan

930 tests pass, 3 skipped
Submit a scorecard with a #N/A formula error — verify no data written to DB and user sees detailed error
Submit a valid scorecard — verify normal flow still works end-to-end

Closes #78
Supersedes #79

🤖 Generated with Claude Code

## Summary - Read all spreadsheet data (plays, box score, pitching decisions) **before** any database writes, preventing partial commits when the spreadsheet has formula errors (e.g. `#N/A`) - Preserve detailed `SheetsException` messages through the error chain instead of wrapping them in generic "Unable to read pitching decisions" - Add dedicated `SheetsException` handler that shows users the specific cell and error type instead of "An unexpected error occurred" ## Context On 2026-03-10, a CLS vs MKE scorecard submission failed because the Pitcherstats tab had a `#N/A` formula error in `team_id` (Row 4). The old code had already committed 74 plays and updated the game score before attempting to read pitching decisions — leaving the DB in a partial state. The user saw only: "An unexpected error occurred. Please try again or contact an admin." ## What changed - **`commands/league/submit_scorecard.py`**: Moved all three spreadsheet reads into a single Phase 6 before any DB writes (Phase 7-9). Added `except SheetsException` handler with user-friendly detailed message. - **`services/sheets_service.py`**: Added `except SheetsException: raise` in `read_pitching_decisions` and `read_box_score` so detailed error messages aren't swallowed by the generic wrapper. ## Test plan - [x] 930 tests pass, 3 skipped - [ ] Submit a scorecard with a `#N/A` formula error — verify no data written to DB and user sees detailed error - [ ] Submit a valid scorecard — verify normal flow still works end-to-end Closes #78 Supersedes #79 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 1 commit 2026-03-11 01:55:37 +00:00

fix: prevent partial DB writes and show detailed errors on scorecard submission failure 461112b2f8

Read all spreadsheet data (plays, box score, pitching decisions) before any
database writes so formula errors like #N/A don't leave the DB in a partial
state. Also preserve SheetsException detail through the error chain and show
users the specific cell/error instead of a generic failure message.

Closes #78

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

Claude added the

ai-reviewing

label 2026-03-11 02:15:47 +00:00

Claude reviewed 2026-03-11 02:18:16 +00:00

Claude left a comment

AI Code Review

Files Reviewed

commands/league/submit_scorecard.py (modified)
services/sheets_service.py (modified)

Findings

Correctness

Main fix is correct. All three spreadsheet reads (read_playtable_data, read_box_score, read_pitching_decisions) are now consolidated into Phase 6 before any DB writes. This eliminates the root cause of the 2026-03-10 incident where 74 plays and a game score were committed before the Pitcherstats #N/A error surfaced.
read_pitching_decisions already raises SheetsException directly (at sheets_service.py:372) when it detects a formula error. Without the except SheetsException: raise guard, that exception was being caught by the generic except Exception handler and re-raised as the opaque "Unable to read pitching decisions" — losing the cell location and error type. The fix is correct.
read_box_score SheetsException guard is defensive but harmless. read_box_score doesn't internally raise SheetsException today, so the guard is a no-op that adds consistency and future-proofing.
WP/LP validation moved correctly. The old code validated pitchers after GAME_PATCHED and needed a rollback; the new code validates in Phase 6 before any writes, so the rollback is correctly omitted.
SheetsException is already imported in submit_scorecard.py (from exceptions import SheetsException, APIException). No import issues.

Security

No issues. This change reorders read/write operations; it doesn't touch auth, user input sanitization, or data exposure.

Style & Conventions

Follows project patterns: self.logger.error(..., error=e), await interaction.edit_original_response(...), structured phase comments.
No double-emoji in embed titles, no get_client() calls outside service layer.

Suggestions

Dead rollback code in SheetsException handler. With all sheet reads now in Phase 6, a SheetsException can only be raised before any DB write, meaning rollback_state is always None when this handler fires. The if rollback_state and game_id: block (submit_scorecard.py:358-365) is unreachable dead code. It's harmless and defensive (guards against future regressions if someone adds a sheet read after DB writes), but it may be confusing to readers who expect the handler to actually roll back. Worth a comment like # Note: SheetsException is only raised during Phase 6 reads (before any DB writes), so rollback_state is always None here. — or simply remove it for clarity.
Phase numbering gap. After the PR, phase labels in the code jump from 9 (POST Decisions) to 12 (Create Results Embed), skipping 10–11. These labels are cosmetic, but the gap may be confusing during future maintenance. Could renumber to 10, 11, 12 or just remove the phase labels that are now out of order.
read_playtable_data doesn't need the SheetsException guard (it never raises one internally), so its absence is correct — no action needed.

Verdict: COMMENT

The fix is correct, complete, and directly addresses the production incident. No bugs, no security issues, follows all project conventions. Suggestions above are cosmetic/defensive and don't block merging.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `commands/league/submit_scorecard.py` (modified) - `services/sheets_service.py` (modified) ### Findings #### Correctness - **Main fix is correct.** All three spreadsheet reads (`read_playtable_data`, `read_box_score`, `read_pitching_decisions`) are now consolidated into Phase 6 before any DB writes. This eliminates the root cause of the 2026-03-10 incident where 74 plays and a game score were committed before the Pitcherstats `#N/A` error surfaced. - **`read_pitching_decisions` already raises `SheetsException` directly** (at `sheets_service.py:372`) when it detects a formula error. Without the `except SheetsException: raise` guard, that exception was being caught by the generic `except Exception` handler and re-raised as the opaque `"Unable to read pitching decisions"` — losing the cell location and error type. The fix is correct. - **`read_box_score` `SheetsException` guard is defensive but harmless.** `read_box_score` doesn't internally raise `SheetsException` today, so the guard is a no-op that adds consistency and future-proofing. - **WP/LP validation moved correctly.** The old code validated pitchers after `GAME_PATCHED` and needed a rollback; the new code validates in Phase 6 before any writes, so the rollback is correctly omitted. - **`SheetsException` is already imported** in `submit_scorecard.py` (`from exceptions import SheetsException, APIException`). No import issues. #### Security - No issues. This change reorders read/write operations; it doesn't touch auth, user input sanitization, or data exposure. #### Style & Conventions - Follows project patterns: `self.logger.error(..., error=e)`, `await interaction.edit_original_response(...)`, structured phase comments. - No double-emoji in embed titles, no `get_client()` calls outside service layer. #### Suggestions 1. **Dead rollback code in `SheetsException` handler.** With all sheet reads now in Phase 6, a `SheetsException` can only be raised before any DB write, meaning `rollback_state` is always `None` when this handler fires. The `if rollback_state and game_id:` block (`submit_scorecard.py:358-365`) is unreachable dead code. It's harmless and defensive (guards against future regressions if someone adds a sheet read after DB writes), but it may be confusing to readers who expect the handler to actually roll back. Worth a comment like `# Note: SheetsException is only raised during Phase 6 reads (before any DB writes), so rollback_state is always None here.` — or simply remove it for clarity. 2. **Phase numbering gap.** After the PR, phase labels in the code jump from 9 (`POST Decisions`) to 12 (`Create Results Embed`), skipping 10–11. These labels are cosmetic, but the gap may be confusing during future maintenance. Could renumber to 10, 11, 12 or just remove the phase labels that are now out of order. 3. **`read_playtable_data` doesn't need the `SheetsException` guard** (it never raises one internally), so its absence is correct — no action needed. ### Verdict: COMMENT The fix is correct, complete, and directly addresses the production incident. No bugs, no security issues, follows all project conventions. Suggestions above are cosmetic/defensive and don't block merging. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-11 02:18:27 +00:00

cal merged commit 514797b787 into next-release

2026-03-11 16:27:29 +00:00

cal deleted branch fix/scorecard-submission-resilience-v2

2026-03-11 16:27:29 +00:00

cal referenced this issue from a commit

2026-03-11 16:27:30 +00:00

Merge pull request 'fix: prevent partial DB writes on scorecard submission failure' (#80) from fix/scorecard-submission-resilience-v2 into next-release