johnsturgeon/tgfp-web

Watch

Fork 0

Restore award recalculation #423

Merged

johnsturgeon merged 5 commits from fix-awards-never-recalculated-422 into main

2026-08-25 14:00:18 +02:00

johnsturgeon commented

2026-08-25 09:46:44 +02:00

Owner

update_all_awards() lost every caller when scheduling moved to the
dispatcher, so no award has been recalculated and no award notification has
reached Discord since that release. Production surfaced it on restart, as
APScheduler dropping the last stale jobstore row:

Unable to restore job "update_awards" -- removing it
LookupError: Error resolving reference
app.jobs.award_update_all:scheduled_update_all_awards: error looking up object

Nothing was lost — awards are recomputed from picks and games on every run, so
they reappear on the first pass after this ships.

Changes

award_update_all.py — update_all_awards() takes a Session instead
of opening one, matching update_player_records.
scheduled_update_all_awards() is the zero-arg entry point the scheduler
needs.
sync_scores_for_current_week.py — calls it on the newly_final
transition, after update_player_records. This is the primary path: a game
going final is the only mid-week event that can earn an award, and
send_award_notification() fires from the tail of the same call.
dispatcher.py — _schedule_update_all_awards(), Tue 5:30am PT, guarded
like _schedule_sync_team_records. Backstop for slates that finish while the
app is down.
test_integrity.py — every add_job("module:function", ...) string under
app/ must name a module that exists and defines that function. APScheduler
resolves these at fire time, so a rename fails in production rather than CI.

Testing

145 passed, pylint 10.00/10, flake8 clean. The new check was verified by
pointing the award job at a nonexistent function and confirming it fails.

Version bumped 2.18.0 → 2.18.1.

resolves #422

🤖 Generated with Claude Code

`update_all_awards()` lost every caller when scheduling moved to the dispatcher, so no award has been recalculated and no award notification has reached Discord since that release. Production surfaced it on restart, as APScheduler dropping the last stale jobstore row: ``` Unable to restore job "update_awards" -- removing it LookupError: Error resolving reference app.jobs.award_update_all:scheduled_update_all_awards: error looking up object ``` Nothing was lost — awards are recomputed from picks and games on every run, so they reappear on the first pass after this ships. ## Changes - **`award_update_all.py`** — `update_all_awards()` takes a `Session` instead of opening one, matching `update_player_records`. `scheduled_update_all_awards()` is the zero-arg entry point the scheduler needs. - **`sync_scores_for_current_week.py`** — calls it on the `newly_final` transition, after `update_player_records`. This is the primary path: a game going final is the only mid-week event that can earn an award, and `send_award_notification()` fires from the tail of the same call. - **`dispatcher.py`** — `_schedule_update_all_awards()`, Tue 5:30am PT, guarded like `_schedule_sync_team_records`. Backstop for slates that finish while the app is down. - **`test_integrity.py`** — every `add_job("module:function", ...)` string under `app/` must name a module that exists and defines that function. APScheduler resolves these at fire time, so a rename fails in production rather than CI. ## Testing 145 passed, pylint 10.00/10, flake8 clean. The new check was verified by pointing the award job at a nonexistent function and confirming it fails. Version bumped 2.18.0 → 2.18.1. resolves #422 🤖 Generated with [Claude Code](https://claude.com/claude-code)

johnsturgeon added 3 commits

2026-08-25 09:46:44 +02:00

Restore award recalculation ca30ce28ac

update_all_awards() lost every caller when scheduling moved to the
dispatcher, so no award has been recalculated and no award notification
has reached Discord since that release. Production surfaced it as
APScheduler dropping the last stale jobstore row on restart.

Awards now update on the newly-final transition in the score sync, which
is the only mid-week event that can earn one, with a weekly dispatcher
cron as the backstop for slates that finish while the app is down.
update_all_awards() takes a session so the score sync recalculates
against the rows it just committed; scheduled_update_all_awards() owns a
session for the scheduler, which cannot pass one.

A static integrity check now fails on any add_job reference that does not
resolve -- the class of breakage that hid this one, since APScheduler
only resolves those at fire time.

resolves #422

adding in the typo fix, also removing comments as I prefer self documenting code 9cc519f2a2

(update all awards when is final. we don't need to say "why")

Cut the caller reference from update_all_awards' docstring

Claude Code Review / claude-review (pull_request) Successful in 1m37s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 31s

Details

Tests / pytest (pull_request) Successful in 14s

Details

97ffc33c19

The signature already says the caller supplies the session, and naming
sync_scores_for_current_week there goes stale the first time this is
called from somewhere else.

forgejo-actions commented

2026-08-25 09:48:20 +02:00

This is a well-constructed PR. Let me verify one last thing about how the awards interact during the newly-final path versus week-state completion, then write the review.

The design is coherent. Here's my review:

Summary

This restores the award-recalculation path that lost all callers when scheduling moved to the dispatcher, wiring it into both the per-game "newly final" transition and a Tuesday backstop cron, and adds an AST integrity test that catches dangling add_job("module:function") references in CI instead of at APScheduler fire time. The change is correct, well-scoped, and the new week-state guards on the week-aggregate awards are exactly right; I found no blocking issues.

The two structural decisions that matter both check out:

Week-aggregate awards (sync_perfect_week, sync_won_the_week) now early-return unless week.game_state == WeekState.ALL_FINAL, so the Thursday-night "everyone is 1-0" false perfect-week is prevented (app/jobs/award_update_all.py:25, :71). The per-game (in_your_face) and first-submitter (quick_pick) awards correctly remain ungated since they can be earned mid-week.
The scores job (1-min cadence) sees a stale IN_PROGRESS week and skips the aggregate awards, while sync_week_state_from_game_status (5-min cadence) flips the state to ALL_FINAL and re-runs the sync — so the aggregate awards land within ~5 minutes and duplicate work is idempotent via the PlayerAward unique constraint and notified_at dedup. Coverage is complete.

Worth fixing

Award-sync ordering can silently drop the ungated awards. In _sync_week_awards (app/jobs/award_update_all.py:105), sync_perfect_week runs first and raises AwardSyncException("Too few players") when fewer than 2 active players exist. The surrounding try/except catches it and skips the remaining three syncs — including sync_quick_pick and sync_in_your_face, which have no minimum-player requirement. In a real pool this never fires, but the ordering couples unrelated awards to a check that only two of the four care about. Consider making each sync self-contained (catch per-sync, or move the "too few players" checks so they only skip their own award).

Nits

Concurrent award syncs can raise on a rare insert race. sync_scores_for_current_week (1-min) and sync_week_state_from_game_status (5-min) are distinct APScheduler jobs and can overlap. upsert_award_with_args does a select-then-insert and commits per-award, so a genuinely concurrent grant of the same award would hit the uq_playeraward_player_award_week constraint and error the losing job. It's self-healing (next run recomputes) and low-probability, but it'll show up as Sentry noise once the season is live.
Test coverage gap on the transition wiring. tests/test_award_sync.py nicely covers the guard logic in _sync_week_awards, but nothing exercises the week_completed transition detection in sync_week_state_from_game_status.py:47-54 (the != ALL_FINAL → == ALL_FINAL edge, and that re-running when already ALL_FINAL does not re-fire). A small test there would lock in the "fires exactly once per completion" contract.
Good, necessary fix hiding in the diff: adding sqlite_where to the partial lock index (app/models/player_game_pick.py:79) is what lets the new tests insert multiple non-lock picks per player/week under SQLite; without it SQLAlchemy would emit a full unique index on that dialect. Worth calling out in the PR description since it's a correctness fix for the test DB, not just cosmetic. Production (Postgres) is unaffected, so no migration is needed — correct.

The integrity test (tests/test_integrity.py) is a nice touch and directly addresses the root cause of #422; the "guard the guard" test_job_references_exist is the right instinct so the scan can't pass vacuously.

This is a well-constructed PR. Let me verify one last thing about how the awards interact during the newly-final path versus week-state completion, then write the review. The design is coherent. Here's my review: --- ## Summary This restores the award-recalculation path that lost all callers when scheduling moved to the dispatcher, wiring it into both the per-game "newly final" transition and a Tuesday backstop cron, and adds an AST integrity test that catches dangling `add_job("module:function")` references in CI instead of at APScheduler fire time. The change is correct, well-scoped, and the new week-state guards on the week-aggregate awards are exactly right; I found no blocking issues. The two structural decisions that matter both check out: - Week-aggregate awards (`sync_perfect_week`, `sync_won_the_week`) now early-return unless `week.game_state == WeekState.ALL_FINAL`, so the Thursday-night "everyone is 1-0" false perfect-week is prevented (`app/jobs/award_update_all.py:25`, `:71`). The per-game (`in_your_face`) and first-submitter (`quick_pick`) awards correctly remain ungated since they can be earned mid-week. - The scores job (1-min cadence) sees a stale `IN_PROGRESS` week and skips the aggregate awards, while `sync_week_state_from_game_status` (5-min cadence) flips the state to `ALL_FINAL` and re-runs the sync — so the aggregate awards land within ~5 minutes and duplicate work is idempotent via the `PlayerAward` unique constraint and `notified_at` dedup. Coverage is complete. ## Worth fixing - **Award-sync ordering can silently drop the ungated awards.** In `_sync_week_awards` (`app/jobs/award_update_all.py:105`), `sync_perfect_week` runs first and raises `AwardSyncException("Too few players")` when fewer than 2 active players exist. The surrounding `try/except` catches it and skips the remaining three syncs — including `sync_quick_pick` and `sync_in_your_face`, which have no minimum-player requirement. In a real pool this never fires, but the ordering couples unrelated awards to a check that only two of the four care about. Consider making each sync self-contained (catch per-sync, or move the "too few players" checks so they only skip their own award). ## Nits - **Concurrent award syncs can raise on a rare insert race.** `sync_scores_for_current_week` (1-min) and `sync_week_state_from_game_status` (5-min) are distinct APScheduler jobs and can overlap. `upsert_award_with_args` does a select-then-insert and commits per-award, so a genuinely concurrent grant of the same award would hit the `uq_playeraward_player_award_week` constraint and error the losing job. It's self-healing (next run recomputes) and low-probability, but it'll show up as Sentry noise once the season is live. - **Test coverage gap on the transition wiring.** `tests/test_award_sync.py` nicely covers the guard logic in `_sync_week_awards`, but nothing exercises the `week_completed` transition detection in `sync_week_state_from_game_status.py:47-54` (the `!= ALL_FINAL → == ALL_FINAL` edge, and that re-running when already `ALL_FINAL` does *not* re-fire). A small test there would lock in the "fires exactly once per completion" contract. - **Good, necessary fix hiding in the diff:** adding `sqlite_where` to the partial lock index (`app/models/player_game_pick.py:79`) is what lets the new tests insert multiple non-lock picks per player/week under SQLite; without it SQLAlchemy would emit a full unique index on that dialect. Worth calling out in the PR description since it's a correctness fix for the test DB, not just cosmetic. Production (Postgres) is unaffected, so no migration is needed — correct. The integrity test (`tests/test_integrity.py`) is a nice touch and directly addresses the root cause of #422; the "guard the guard" `test_job_references_exist` is the right instinct so the scan can't pass vacuously.

johnsturgeon added 1 commit

2026-08-25 10:02:17 +02:00

Scope the award fast path to the week that changed

Claude Code Review / claude-review (pull_request) Successful in 2m22s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 24s

Details

Tests / pytest (pull_request) Successful in 10s

Details

f42d8d2c20

Only the week whose game just went final can gain an award, but the
newly-final path recomputed every week of the season -- roughly thirteen
times on a Sunday, and more expensive the later in the season it runs.
The Tuesday cron keeps the full-season pass, which is where a corrected
result in an earlier week gets picked up.

Award failures also no longer take the score sync down with them. A week
with fewer than two active players raises AwardSyncException, which now
surfaces inside the per-minute score job, where it would stop score and
status ingestion mid-slate over something cosmetic. Catching it per week
also means one bad week does not cost the others.

CLAUDE.md said awards run on startup and after picks submission. Both
callers are gone, so it now describes what actually triggers them.

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

johnsturgeon added 1 commit

2026-08-25 13:32:47 +02:00

Grant week-aggregate awards only on a complete week

Claude Code Review / claude-review (pull_request) Successful in 2m43s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 22s

Details

Tests / pytest (pull_request) Successful in 11s

Details

965ca64e20

Wins and losses count final games only, so mid-week every player who
picked the Thursday winner reads as 1-0 -- exactly the condition
sync_perfect_week awards on. Routing the award sync through the
newly-final transition therefore handed out Perfect Week to most of the
pool every Thursday night, and Won The Week to whoever happened to lead
at the time. Awards are insert-only and nothing ever deletes one, so
those stick, and the Discord webhook congratulates each of them
immediately.

Perfect Week and Won The Week now require the week to be ALL_FINAL. That
state is already maintained from game status, cancelled games included,
so this reuses the existing definition of a finished week rather than
adding a second one. In Your Face and Quick Pick stay on the fast path:
the first needs a final game to have a winner at all, and the second
resolves to the earliest pick, which a later submission cannot displace.

The week-state job now runs the award sync when it observes the
transition into ALL_FINAL, which is what keeps the aggregates from
waiting for the Tuesday backstop. Comparing against the persisted state
rather than an in-memory flag makes that survive a restart, and lets a
rescheduled game re-open and re-close the week correctly.

uq_one_lock_per_week carried only postgresql_where, so under SQLite it
became an unconditional unique index and a player could hold one pick
per week -- the test below could not seed a realistic week without it.

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

johnsturgeon merged commit d4a614e8e2 into main

2026-08-25 14:00:18 +02:00

johnsturgeon deleted branch fix-awards-never-recalculated-422

2026-08-25 14:00:18 +02:00

johnsturgeon referenced this pull request from a commit

2026-08-25 14:00:18 +02:00

Restore award recalculation (#423)