johnsturgeon/tgfp-web

Watch

Fork 0

Tell the pool when the picks page opens #451

Merged

johnsturgeon merged 1 commit from re-automate-the-picks-page-is-ready-discord-chat-210 into main

2026-09-02 18:35:22 +02:00

johnsturgeon commented

2026-09-02 18:19:51 +02:00

Owner

The odds lock moved to Wednesday 6am and nothing announced it, so the picks
page released to an empty room.

Where the announcement goes

Rule 4 is the only place odds_state changes, and _get_current_odds_state is
one way, so the only transition it can make is PRELIMINARY to LOCKED. The
announcement hangs off that transition instead of becoming a rule of its own:

    current_odds_state = _get_current_odds_state(week, now_utc)
    if current_odds_state != week.odds_state:
        week.odds_state = current_odds_state
        session.commit()
        scheduler.launch_announce_picks_open()
        return

Once per week by construction -- the reconciler falsifies its own condition, so
there is no staleness window to tune and nothing added to the rule table.

It could not have been a rule keyed on _is_stale: successes is keyed by
job, not by week, so week 2's announcement would look already-done because week
1's row is sitting there. The nag gets away with that only because it is
time-relative and repeats.

The trade is at-most-once -- commit lands, container dies, nobody is told. If
that ever matters the upgrade is a week.announced_at column and its own rule,
the same shape as PlayerAward.notified_at.

The dispatcher reads as a table again

Rules are grouped by the state each one turns on:

#	rule
1	sync game state	writes `game_state`
2	no games yet, go get them	reads it
3	refresh the game schedule	reads it
4	sync odds state	writes `odds_state`
5	update odds	reads it
6	nag players	reads it
7 / 7b	update scores / catch up	read the games
8 / 9	player records / awards	read the games

Odds locking is now a reconciler shaped exactly like game state, with the
one-way rule expressed in _get_current_odds_state rather than only in the
OddsState docstring.

Rules 8 and 9 ask _game_finished_since_last_run(games, successes, job_id),
which retires the early if last_change is None: return -- a guard that would
have silently disabled any rule added below rule 9 on every week without a
finished game, which is most of every week.

Tests

283 passing. Five odds tests needed a seeded schedule refresh: they had been
relying on rule ordering without saying so, and the regrouping surfaced it.

Mutation-checked, each caught:

mutation	failures
announce dispatch removed	2
announce fires every tick	20
one-way lock guard removed	1
odds sync hoisted above rule 2 (`min()` over no games)	2
`None` guard removed from the finished-game check	1
rule 9 keyed off rule 8's `JobState` row	1

resolves #210

🤖 Generated with Claude Code

The odds lock moved to Wednesday 6am and nothing announced it, so the picks page released to an empty room. ## Where the announcement goes Rule 4 is the only place `odds_state` changes, and `_get_current_odds_state` is one way, so the only transition it can make is PRELIMINARY to LOCKED. The announcement hangs off that transition instead of becoming a rule of its own: ```python current_odds_state = _get_current_odds_state(week, now_utc) if current_odds_state != week.odds_state: week.odds_state = current_odds_state session.commit() scheduler.launch_announce_picks_open() return ``` Once per week by construction -- the reconciler falsifies its own condition, so there is no staleness window to tune and nothing added to the rule table. It could not have been a rule keyed on `_is_stale`: `successes` is keyed by job, not by week, so week 2's announcement would look already-done because week 1's row is sitting there. The nag gets away with that only because it is time-relative and repeats. The trade is at-most-once -- commit lands, container dies, nobody is told. If that ever matters the upgrade is a `week.announced_at` column and its own rule, the same shape as `PlayerAward.notified_at`. ## The dispatcher reads as a table again Rules are grouped by the state each one turns on: | # | rule | | |---|---|---| | 1 | sync game state | **writes `game_state`** | | 2 | no games yet, go get them | reads it | | 3 | refresh the game schedule | reads it | | 4 | sync odds state | **writes `odds_state`** | | 5 | update odds | reads it | | 6 | nag players | reads it | | 7 / 7b | update scores / catch up | read the games | | 8 / 9 | player records / awards | read the games | Odds locking is now a reconciler shaped exactly like game state, with the one-way rule expressed in `_get_current_odds_state` rather than only in the `OddsState` docstring. Rules 8 and 9 ask `_game_finished_since_last_run(games, successes, job_id)`, which retires the early `if last_change is None: return` -- a guard that would have silently disabled any rule added below rule 9 on every week without a finished game, which is most of every week. ## Tests 283 passing. Five odds tests needed a seeded schedule refresh: they had been relying on rule ordering without saying so, and the regrouping surfaced it. Mutation-checked, each caught: | mutation | failures | |---|---| | announce dispatch removed | 2 | | announce fires every tick | 20 | | one-way lock guard removed | 1 | | odds sync hoisted above rule 2 (`min()` over no games) | 2 | | `None` guard removed from the finished-game check | 1 | | rule 9 keyed off rule 8's `JobState` row | 1 | resolves #210 🤖 Generated with [Claude Code](https://claude.com/claude-code)

johnsturgeon added 1 commit

2026-09-02 18:19:51 +02:00

Tell the pool when the picks page opens

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

Details

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

Details

Tests / pytest (pull_request) Successful in 13s

Details

ab3a39a91c

The odds lock moved to Wednesday 6am and nothing announced it, so the page
released to an empty room.

Rule 4 is the only place `odds_state` changes and `_get_current_odds_state` is
one way, so the announcement hangs off that transition rather than becoming a
rule of its own. Nothing to keep track of and no staleness window: it fires
once per week because the reconciler falsifies its own condition.

The rest is the dispatcher reading as a table again. Rules are grouped by the
state each one turns on -- rule 1 fixes `game_state` and 2 and 3 act on it,
rule 4 fixes `odds_state` and 5 and 6 act on it. Odds locking becomes a
reconciler shaped exactly like game state, with the one-way rule stated in
`_get_current_odds_state` rather than only in a docstring. Rules 8 and 9 ask
`_game_finished_since_last_run`, which retires the early return that would have
silently disabled anything added below rule 9.

Code by John. Tests by Claude Opus 5, directed and reviewed by John.

resolves #210

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

forgejo-actions commented

2026-09-02 18:21:30 +02:00

This is a well-structured refactor with genuinely good test coverage; I checked the surrounding code and the core logic is sound.

Review

The change reorganizes tick() into a clean rule table, expresses odds locking as a one-way reconciler (_get_current_odds_state), and hangs the new Discord announcement off the single PRELIMINARY→LOCKED transition. I verified the domain premise — app/main.py:305 gates the picks page on exactly odds_state == LOCKED, so coupling the announcement to that transition is correct — and confirmed rule 4 stays below rule 2, so week.utc_lock_odds_at (which does min() over self.games at app/models/week.py:111) is never evaluated on an empty game list.

Worth fixing

At-most-once delivery is a real gap, not just a documented trade. app/jobs/dispatcher.py:110-114 commits the LOCKED state and only then schedules the announce; if the process dies between the commit and the job running (or the webhook throws), the announcement is lost forever because the reconciler has already falsified its own condition. The PR calls this out and the "empty room" bug that motivated the whole change is precisely a missed announcement — so it's slightly ironic to ship a mechanism that can still silently drop it. The suggested week.announced_at column is the right fix; consider whether it's worth doing now rather than later.

Nits

User-facing typo: app/jobs/announce_picks_page.py:11 — "Announcment" → "Announcement".
Logging side-effect in a builder: _picks_page_ready_message() (announce_picks_page.py:9-14) emits sentry_logger.info(...) as a side effect of a function that otherwise just returns a string. Move the log into announce_picks_page_to_discord() so the builder stays pure.
Lost footgun documentation: the removed nag_players.py module docstring warned that from sentry_sdk import logger as sentry_logger must be imported explicitly or sentry_sdk.logger raises AttributeError. That warning applied process-wide and the new announce_picks_page.py relies on the same idiom; dropping it loses institutional knowledge. Consider relocating a one-line version somewhere durable rather than deleting it outright.
Job body untested: announce_picks_page_to_discord has no direct unit test (webhook mocked). This is consistent with nag_the_players, which also has no body-level test, so it's not a regression — but the dispatcher tests only prove the job is scheduled, not that it constructs a valid webhook.

Module-level Config.get_config() at announce_picks_page.py:6 mirrors the existing pattern in scheduler.py:69, and webhook.execute() runs in the scheduler thread pool rather than an async handler, so neither is a concern.

This is a well-structured refactor with genuinely good test coverage; I checked the surrounding code and the core logic is sound. ## Review The change reorganizes `tick()` into a clean rule table, expresses odds locking as a one-way reconciler (`_get_current_odds_state`), and hangs the new Discord announcement off the single PRELIMINARY→LOCKED transition. I verified the domain premise — `app/main.py:305` gates the picks page on exactly `odds_state == LOCKED`, so coupling the announcement to that transition is correct — and confirmed rule 4 stays below rule 2, so `week.utc_lock_odds_at` (which does `min()` over `self.games` at `app/models/week.py:111`) is never evaluated on an empty game list. ### Worth fixing - **At-most-once delivery is a real gap, not just a documented trade.** `app/jobs/dispatcher.py:110-114` commits the LOCKED state and only then schedules the announce; if the process dies between the commit and the job running (or the webhook throws), the announcement is lost forever because the reconciler has already falsified its own condition. The PR calls this out and the "empty room" bug that motivated the whole change is precisely a missed announcement — so it's slightly ironic to ship a mechanism that can still silently drop it. The suggested `week.announced_at` column is the right fix; consider whether it's worth doing now rather than later. ### Nits - **User-facing typo:** `app/jobs/announce_picks_page.py:11` — "Announcment" → "Announcement". - **Logging side-effect in a builder:** `_picks_page_ready_message()` (`announce_picks_page.py:9-14`) emits `sentry_logger.info(...)` as a side effect of a function that otherwise just returns a string. Move the log into `announce_picks_page_to_discord()` so the builder stays pure. - **Lost footgun documentation:** the removed `nag_players.py` module docstring warned that `from sentry_sdk import logger as sentry_logger` must be imported explicitly or `sentry_sdk.logger` raises `AttributeError`. That warning applied process-wide and the new `announce_picks_page.py` relies on the same idiom; dropping it loses institutional knowledge. Consider relocating a one-line version somewhere durable rather than deleting it outright. - **Job body untested:** `announce_picks_page_to_discord` has no direct unit test (webhook mocked). This is consistent with `nag_the_players`, which also has no body-level test, so it's not a regression — but the dispatcher tests only prove the job is *scheduled*, not that it constructs a valid webhook. Module-level `Config.get_config()` at `announce_picks_page.py:6` mirrors the existing pattern in `scheduler.py:69`, and `webhook.execute()` runs in the scheduler thread pool rather than an async handler, so neither is a concern.

johnsturgeon merged commit 255f40d4a9 into main

2026-09-02 18:35:22 +02:00

johnsturgeon deleted branch re-automate-the-picks-page-is-ready-discord-chat-210

2026-09-02 18:35:22 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-02 18:35:23 +02:00

Merge pull request 'Tell the pool when the picks page opens' (#451) from re-automate-the-picks-page-is-ready-discord-chat-210 into main