Watch

The quick / easy solution is to just make sure we're updating the player records on a schedule #495

Merged

johnsturgeon merged 5 commits from a-player-putting-their-picks-in-after-games-have-auto-started-do-not-re-calculate-their-standings-434 into main

2026-09-07 16:35:05 +02:00

johnsturgeon commented

2026-09-07 14:06:12 +02:00

Owner

It's a DB only operation so running it hourly is just fine.

Fixed a typo along the way

Resolves #434

It's a DB only operation so running it hourly is just fine. - Fixed a typo along the way Resolves #434

johnsturgeon added 1 commit

2026-09-07 14:06:12 +02:00

The quick / easy solution is to just make sure we're updating the player records on a schedule

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

Details

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

Details

Tests / pytest (pull_request) Successful in 13s

Details

97c4207ad6

It's a DB only operation so running it hourly is just fine.
- Fixed a typo along the way

Resolves #434

forgejo-actions commented

2026-09-07 14:07:44 +02:00

This is a clean, low-risk change. I've reviewed the scheduler wiring, the new job, the dispatcher interaction, and the test-guard changes. Here's my review.

Review

This PR adds an hourly update_player_records job and, in passing, converts most add_job calls from dotted-string refs to direct callables plus adds next_run_time to the awards job for consistent startup behavior. The core change is sound and the test-guard rewrite correctly reflects the new "callables everywhere except the dispatcher" convention; I found no blocking issues.

Worth fixing

Undocumented behavior change in schedule_update_all_awards (app/jobs/scheduler.py:182). The diff adds next_run_time=datetime.now(timezone.utc) to the awards job, which is not mentioned in the PR description (it only claims player records + a typo). The effect is that a full update_all_awards recalculation now fires on every process restart/deploy, in addition to the Tuesday cron. This appears intentional given the 2cc4d51 commit message ("running all scheduled jobs once on startup"), and it's consistent with the other interval jobs — but it's a heavier operation than a player-record rebuild, so it's worth confirming that's desired and calling it out in the PR description rather than shipping it silently under a "player records" title.

Nits

Two job IDs can now run update_player_records concurrently. The dispatcher already launches it via launch_update_player_records() (id UPDATE_PLAYER_RECORDS, app/jobs/dispatcher.py:155), and this adds a second registration under SCHEDULED_UPDATE_PLAYER_RECORDS (app/jobs/scheduler.py:186). APScheduler's max_instances=1 is per-job-id, so a game finishing on the hour could run both at once. recalculate_record is a full idempotent rebuild (app/models/player.py:110), so the worst case is duplicated work / transient row contention rather than corruption — safe, but worth a one-line comment noting the overlap is intentional and benign.
Hourly scheduling amplifies a pre-existing N+1. update_player_records calls recalculate_record per active player (app/jobs/update_player_records.py:9-10), and each call re-runs Week.all_weeks_for_current_season(session) (app/models/player.py:114). That per-player week query isn't introduced here, but this PR puts it on an unconditional hourly cadence for all active players. Hoisting the week-id set out of the loop would be a cheap follow-up. Out of scope for this PR.
No test asserts the new job is wired up. test_integrity.py only guards string-ref resolution, so neither the new schedule_update_player_records registration nor its inclusion in lifespan (app/main.py:127) is covered by any test. Consistent with the existing schedule_* functions (none are tested either), so this is a pre-existing gap, not a regression — just flagging it since the whole point of #422's guard was to catch scheduler drift.

The string-ref → callable conversion is actually a net improvement for safety: callables are resolved at scheduler.py import time, so a rename now fails in CI on import rather than at fire time in production, and the dispatcher (the one genuine import-cycle case) correctly stays a string and remains covered by test_scheduled_jobs_resolve.

This is a clean, low-risk change. I've reviewed the scheduler wiring, the new job, the dispatcher interaction, and the test-guard changes. Here's my review. --- ## Review This PR adds an hourly `update_player_records` job and, in passing, converts most `add_job` calls from dotted-string refs to direct callables plus adds `next_run_time` to the awards job for consistent startup behavior. The core change is sound and the test-guard rewrite correctly reflects the new "callables everywhere except the dispatcher" convention; I found no blocking issues. ### Worth fixing - **Undocumented behavior change in `schedule_update_all_awards` (`app/jobs/scheduler.py:182`).** The diff adds `next_run_time=datetime.now(timezone.utc)` to the *awards* job, which is not mentioned in the PR description (it only claims player records + a typo). The effect is that a full `update_all_awards` recalculation now fires on **every process restart/deploy**, in addition to the Tuesday cron. This appears intentional given the `2cc4d51` commit message ("running all scheduled jobs once on startup"), and it's consistent with the other interval jobs — but it's a heavier operation than a player-record rebuild, so it's worth confirming that's desired and calling it out in the PR description rather than shipping it silently under a "player records" title. ### Nits - **Two job IDs can now run `update_player_records` concurrently.** The dispatcher already launches it via `launch_update_player_records()` (id `UPDATE_PLAYER_RECORDS`, `app/jobs/dispatcher.py:155`), and this adds a second registration under `SCHEDULED_UPDATE_PLAYER_RECORDS` (`app/jobs/scheduler.py:186`). APScheduler's `max_instances=1` is per-job-id, so a game finishing on the hour could run both at once. `recalculate_record` is a full idempotent rebuild (`app/models/player.py:110`), so the worst case is duplicated work / transient row contention rather than corruption — safe, but worth a one-line comment noting the overlap is intentional and benign. - **Hourly scheduling amplifies a pre-existing N+1.** `update_player_records` calls `recalculate_record` per active player (`app/jobs/update_player_records.py:9-10`), and each call re-runs `Week.all_weeks_for_current_season(session)` (`app/models/player.py:114`). That per-player week query isn't introduced here, but this PR puts it on an unconditional hourly cadence for all active players. Hoisting the week-id set out of the loop would be a cheap follow-up. Out of scope for this PR. - **No test asserts the new job is wired up.** `test_integrity.py` only guards string-ref resolution, so neither the new `schedule_update_player_records` registration nor its inclusion in `lifespan` (`app/main.py:127`) is covered by any test. Consistent with the existing `schedule_*` functions (none are tested either), so this is a pre-existing gap, not a regression — just flagging it since the whole point of #422's guard was to catch scheduler drift. The string-ref → callable conversion is actually a net improvement for safety: callables are resolved at `scheduler.py` import time, so a rename now fails in CI on import rather than at fire time in production, and the dispatcher (the one genuine import-cycle case) correctly stays a string and remains covered by `test_scheduled_jobs_resolve`.

johnsturgeon added 1 commit

2026-09-07 14:11:56 +02:00

addressed claude review feedback

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

Details

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

Details

Tests / pytest (pull_request) Successful in 15s

Details

98e5b59321

johnsturgeon added 1 commit

2026-09-07 14:39:32 +02:00