We now have the week table, which is being written / read properly #413

Merged

johnsturgeon merged 1 commit from phase-1-make-the-week-table-real-407 into main

2026-08-22 10:10:24 +02:00

johnsturgeon commented

2026-08-22 08:57:29 +02:00

Owner

Move WeekInfo into its own module (app/models/week_info.py) to
break the week.py / model_helpers cycle. model_helpers.py is gone.
Thread a session to the Week.current() call sites — dependency.py,
admin.py x2, nag_players.py, scheduler.py, update_all_scores.py,
game.py.
Drop active_week and uq_week_active plus a migration. See
No pointer column below. Confirm the revision drops the column, not
just the index.
Write app/jobs/sync_current_week.py. On an interval, derive the
current week from the games endpoint and get-or-create the Week row
for it. Expose it on an admin route so it can be run by hand.
Schedule it — on an interval, plus one run at startup before
schedule_jobs.
Delete the hardcoded body of Week.current() and query for real.
Remove the # pylint: disable=unused-argument with it.

Resolves #407

- [x] **Move `WeekInfo` into its own module** (`app/models/week_info.py`) to break the `week.py` / `model_helpers` cycle. `model_helpers.py` is gone. - [x] **Thread a session to the `Week.current()` call sites** — `dependency.py`, `admin.py` x2, `nag_players.py`, `scheduler.py`, `update_all_scores.py`, `game.py`. - [x] **Drop `active_week` and `uq_week_active`** plus a migration. See *No pointer column* below. Confirm the revision drops the column, not just the index. - [x] **Write `app/jobs/sync_current_week.py`.** On an interval, derive the current week from the games endpoint and get-or-create the `Week` row for it. Expose it on an admin route so it can be run by hand. - [x] **Schedule it** — on an interval, plus one run at startup before `schedule_jobs`. - [x] **Delete the hardcoded body of `Week.current()`** and query for real. Remove the `# pylint: disable=unused-argument` with it. Resolves #407

johnsturgeon added 1 commit

2026-08-22 08:57:29 +02:00

We now have the week table, which is being written / read properly

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

Details

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

Details

Tests / pytest (pull_request) Successful in 16s

Details

1b63b481bc

- [x] **Move `WeekInfo` into its own module** (`app/models/week_info.py`) to
      break the `week.py` / `model_helpers` cycle. `model_helpers.py` is gone.
- [x] **Thread a session to the `Week.current()` call sites** — `dependency.py`,
      `admin.py` x2, `nag_players.py`, `scheduler.py`, `update_all_scores.py`,
      `game.py`.
- [x] **Drop `active_week` and `uq_week_active`** plus a migration. See
      *No pointer column* below. Confirm the revision drops the column, not
      just the index.
- [x] **Write `app/jobs/sync_current_week.py`.** On an interval, derive the
      current week from the games endpoint and get-or-create the `Week` row
      for it. Expose it on an admin route so it can be run by hand.
- [x] **Schedule it** — on an interval, plus one run at startup before
      `schedule_jobs`.
- [x] **Delete the hardcoded body of `Week.current()`** and query for real.
      Remove the `# pylint: disable=unused-argument` with it.

forgejo-actions commented

2026-08-22 08:59:31 +02:00

This is a clean, well-documented refactor. I've verified the import graph (no lingering model_helpers/current_week_info/active_week references), the NflApiInfo shape (season/season_type/week_no all present), the SeasonType ordering that Week.current() relies on, and the migration. Here's my review.

The PR cleanly replaces the hardcoded current_week_info() with a real Week.current(session) reader backed by a new sync_current_week writer job, breaks the week.py/model_helpers import cycle by extracting WeekInfo, and drops the now-unused active_week column with a reversible migration. The design rationale is documented thoroughly in both code comments and TECH_DEBT.md, and I found no correctness regressions in the wiring — the findings below are a test gap and two smaller robustness points.

Worth fixing

No test coverage for any of the new logic (app/models/week.py:33, app/jobs/sync_current_week.py, app/uptime_kuma.py). This is a new source-of-truth writer plus a reader that raises on empty tables, and the suite runs fully offline from fixtures — so the high-value cases are all cheaply testable: Week.current() returning the max row across a season/season_type/week_no boundary, NoCurrentWeekException on an empty table, sync_current_week's get-or-create idempotency (no duplicate row on a second run), and the ping_kuma up/down/swallow paths. Given the whole point of the PR is that the week table is now "written / read properly," landing it with zero tests on that path is the main gap.
TOCTOU race in sync_current_week (app/jobs/sync_current_week.py:45-62). The existence check and the insert aren't atomic. The scheduled interval job and a hand-triggered /admin/job_sync_current_week (a route in the threadpool, distinct from the scheduler executor) can both pass .first() is None and both INSERT the same (season, season_type, week_no); uq_week_season_type_no then makes the second commit() raise IntegrityError. No data is corrupted (the constraint does its job), but the exception propagates through ping_kuma, which pushes a spurious "down" heartbeat and re-raises into Sentry — i.e. a false alarm on the very monitor this PR adds. Catching IntegrityError around the commit and treating it as a no-op (the row now exists) would make the get-or-create actually concurrency-safe, matching what the comment on line 39-40 already claims it does.

Nits

Fresh-DB availability window is wider than "brief" when Tank01 is unreachable at boot (app/main.py:129-141, app/dependency.py:19). TECH_DEBT.md frames the NoCurrentWeekException → 500 window as lasting only "before the first successful sync," but because the startup sync is a date-triggered job that can fail against a provider outage, on a genuinely empty database the next attempt is the hourly interval — so every page can 500 for up to an hour, not just momentarily. This is an accepted tradeoff and only bites a first-ever boot, but the doc undersells the worst case; a sentence acknowledging the Tank01-down-at-first-boot path would be accurate.
WeekInfo imported through award_helpers (app/jobs/award_update_all.py:20). from app.models.award_helpers import upsert_award_with_args, WeekInfo works (award_helpers re-exports it), but now that WeekInfo is a first-class app.models export, from app.models import ..., WeekInfo would be the more direct source and match how the other touched modules were updated in this same PR.

This is a clean, well-documented refactor. I've verified the import graph (no lingering `model_helpers`/`current_week_info`/`active_week` references), the `NflApiInfo` shape (`season`/`season_type`/`week_no` all present), the `SeasonType` ordering that `Week.current()` relies on, and the migration. Here's my review. --- The PR cleanly replaces the hardcoded `current_week_info()` with a real `Week.current(session)` reader backed by a new `sync_current_week` writer job, breaks the `week.py`/`model_helpers` import cycle by extracting `WeekInfo`, and drops the now-unused `active_week` column with a reversible migration. The design rationale is documented thoroughly in both code comments and `TECH_DEBT.md`, and I found no correctness regressions in the wiring — the findings below are a test gap and two smaller robustness points. ## Worth fixing - **No test coverage for any of the new logic** (`app/models/week.py:33`, `app/jobs/sync_current_week.py`, `app/uptime_kuma.py`). This is a new source-of-truth writer plus a reader that raises on empty tables, and the suite runs fully offline from fixtures — so the high-value cases are all cheaply testable: `Week.current()` returning the max row across a season/season_type/week_no boundary, `NoCurrentWeekException` on an empty table, `sync_current_week`'s get-or-create idempotency (no duplicate row on a second run), and the `ping_kuma` up/down/swallow paths. Given the whole point of the PR is that the week table is now "written / read properly," landing it with zero tests on that path is the main gap. - **TOCTOU race in `sync_current_week`** (`app/jobs/sync_current_week.py:45-62`). The existence check and the insert aren't atomic. The scheduled interval job and a hand-triggered `/admin/job_sync_current_week` (a route in the threadpool, distinct from the scheduler executor) can both pass `.first() is None` and both `INSERT` the same `(season, season_type, week_no)`; `uq_week_season_type_no` then makes the second `commit()` raise `IntegrityError`. No data is corrupted (the constraint does its job), but the exception propagates through `ping_kuma`, which pushes a spurious "down" heartbeat and re-raises into Sentry — i.e. a false alarm on the very monitor this PR adds. Catching `IntegrityError` around the commit and treating it as a no-op (the row now exists) would make the get-or-create actually concurrency-safe, matching what the comment on line 39-40 already claims it does. ## Nits - **Fresh-DB availability window is wider than "brief" when Tank01 is unreachable at boot** (`app/main.py:129-141`, `app/dependency.py:19`). `TECH_DEBT.md` frames the `NoCurrentWeekException` → 500 window as lasting only "before the first successful sync," but because the startup sync is a date-triggered job that can fail against a provider outage, on a genuinely empty database the next attempt is the hourly interval — so every page can 500 for up to an hour, not just momentarily. This is an accepted tradeoff and only bites a first-ever boot, but the doc undersells the worst case; a sentence acknowledging the Tank01-down-at-first-boot path would be accurate. - **`WeekInfo` imported through `award_helpers`** (`app/jobs/award_update_all.py:20`). `from app.models.award_helpers import upsert_award_with_args, WeekInfo` works (award_helpers re-exports it), but now that `WeekInfo` is a first-class `app.models` export, `from app.models import ..., WeekInfo` would be the more direct source and match how the other touched modules were updated in this same PR.

johnsturgeon merged commit c3af9c2a8f into main

2026-08-22 10:10:24 +02:00

johnsturgeon referenced this pull request from a commit

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

We now have the week table, which is being written / read properly (#413)