Sweep TECH_DEBT entries the control loop and #440 resolved #441

Merged

johnsturgeon merged 1 commit from sweep-tech-debt-resolved-by-the-control-loop-436 into main

2026-09-01 18:55:02 +02:00

johnsturgeon commented

2026-09-01 18:51:28 +02:00

Owner

Removed (8)

Code that no longer exists:

Entry	Why
`create_the_picks` runs three times for postseason week 4	`create_picks.py` gone; rewritten as a provider entry, see below
`sync_team_records` dies whole if one team is missing	replaced by `update_team_records`, which warns per team
`schedule_jobs` no longer runs at startup	the scheduler rewrite it deferred to is the dispatcher
A third copy of the all-final calculation	`award_update_all.py` gone
Parse failures carry no context	`NflApiGame` gone; `schemas.py` validates at the edge
Unguarded `[0]` on team lookups	same module
Two provider modules coexist	there is one
`DataSource` outlived its column	`app/models/data_source.py` gone, no references

And one fixed rather than deleted: Pick validity is enforced only in the UI,
closed by the server-side upset check in #440.

Rewritten (3)

No coverage outside the provider wrappers → Routes and templates are
largely untested. test_picks_routes covers the picks page and form and
establishes the harness; the entry now names what is still uncovered
(/standings, /allpicks, v2 admin, mail, templates) and notes that the
expensive part is done.

create_the_picks runs three times → post 4 sticks for months, in the
provider section. The Tank01 behaviour is permanent and worth recording. What
changed is that it is now harmless for three unrelated reasons --
add_week_if_new, GameState leaving NO_GAMES, and the picks page being
gated on OddsState.LOCKED. None of them was written for this, so the entry
says so before someone simplifies one away.

No bootstrap path keeps its analysis but drops the speculative ending for
what actually happened: the Aug 2026 production rebuild restored team only
because rebuild_database.sh was edited to preserve it partway through.

Added (3)

picks_form swallows every IntegrityError (#438) -- three defects
presented as successes during the picks work. All fixed upstream, which is
why the next one will be just as quiet.
The Tank01 retry path has no coverage (#437) -- lost with
test_tank01_api.py.
A slow job can make the dispatcher's next tick fail -- the launchers use
a fixed id with no replace_existing, so a job outliving the 60-second tick
makes the next add_job raise ConflictingIdError. Self-healing, but costs
a tick and logs a misleading error.

Structure

The Migration in flight (provider boundary) section is gone -- the migration
landed. Its two surviving entries moved to Data and schema. Added an Error
handling section for the last one.

Every file path and symbol referenced in the document is verified to exist.

resolves #436

## Removed (8) Code that no longer exists: | Entry | Why | |---|---| | `create_the_picks` runs three times for postseason week 4 | `create_picks.py` gone; rewritten as a provider entry, see below | | `sync_team_records` dies whole if one team is missing | replaced by `update_team_records`, which warns per team | | `schedule_jobs` no longer runs at startup | the scheduler rewrite it deferred to is the dispatcher | | A third copy of the all-final calculation | `award_update_all.py` gone | | Parse failures carry no context | `NflApiGame` gone; `schemas.py` validates at the edge | | Unguarded `[0]` on team lookups | same module | | Two provider modules coexist | there is one | | `DataSource` outlived its column | `app/models/data_source.py` gone, no references | And one fixed rather than deleted: **Pick validity is enforced only in the UI**, closed by the server-side upset check in #440. ## Rewritten (3) **No coverage outside the provider wrappers** → *Routes and templates are largely untested*. `test_picks_routes` covers the picks page and form and establishes the harness; the entry now names what is still uncovered (`/standings`, `/allpicks`, `v2` admin, mail, templates) and notes that the expensive part is done. **`create_the_picks` runs three times** → *`post 4` sticks for months*, in the provider section. The Tank01 behaviour is permanent and worth recording. What changed is that it is now harmless for three unrelated reasons -- `add_week_if_new`, `GameState` leaving `NO_GAMES`, and the picks page being gated on `OddsState.LOCKED`. None of them was written for this, so the entry says so before someone simplifies one away. **No bootstrap path** keeps its analysis but drops the speculative ending for what actually happened: the Aug 2026 production rebuild restored `team` only because `rebuild_database.sh` was edited to preserve it partway through. ## Added (3) - **`picks_form` swallows every `IntegrityError`** (#438) -- three defects presented as successes during the picks work. All fixed upstream, which is why the next one will be just as quiet. - **The Tank01 retry path has no coverage** (#437) -- lost with `test_tank01_api.py`. - **A slow job can make the dispatcher's next tick fail** -- the launchers use a fixed id with no `replace_existing`, so a job outliving the 60-second tick makes the next `add_job` raise `ConflictingIdError`. Self-healing, but costs a tick and logs a misleading error. ## Structure The *Migration in flight (provider boundary)* section is gone -- the migration landed. Its two surviving entries moved to *Data and schema*. Added an *Error handling* section for the last one. Every file path and symbol referenced in the document is verified to exist. resolves #436

johnsturgeon added 1 commit

2026-09-01 18:51:28 +02:00

Sweep TECH_DEBT entries the control loop and #440 resolved

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

Details

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

Details

Tests / pytest (pull_request) Successful in 12s

Details

d4e4e38c22

Eight entries described code that no longer exists: the old provider module and
its two parse-failure entries, `sync_team_records`, `create_picks`, the stale
all-final duplicate, `DataSource`, and the startup-scheduling gap the dispatcher
rewrite was deferring to. `Pick validity is enforced only in the UI` went with
the server-side upset check in #440.

Three entries rewritten rather than removed. Route coverage now says what is
still uncovered instead of claiming there is none. `create_the_picks runs three
times` becomes a provider entry: the Tank01 quirk it described is permanent,
but three unrelated mechanisms now make it harmless, and that is worth writing
down before someone simplifies one of them away.

Two added from the picks work: the `IntegrityError` swallow and the missing
retry-path coverage, both already tracked as issues. One added from reading the
dispatcher: a job that outlives the tick interval makes the next `add_job`
raise `ConflictingIdError`.

The "Migration in flight" section is gone -- the migration landed. Its two
surviving entries moved to Data and schema.

resolves #436

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

forgejo-actions commented

2026-09-01 18:53:18 +02:00

This is a documentation-only PR (plus the routine patch version bump), so I verified the debt entries against the actual code rather than looking for runtime bugs. The sweep is accurate almost everywhere I checked — but one carried-over entry names a job that no longer works the way the entry describes.

Review

A clean, well-organized sweep of docs/TECH_DEBT.md that removes entries for code that no longer exists and rewrites/adds three that reflect the current state. I spot-checked the file paths, symbols, and behavioral claims against the tree; the overwhelming majority hold, but the picks_for_week entry misattributes the fan-out cost to a job that has since stopped using that code path.

Worth fixing

docs/TECH_DEBT.md — the picks_for_week lost its session-scoped cache entry misdescribes update_player_records. The entry claims:

The call sites that could justify it are the fan-out ones: update_player_records and update_awards both loop every active player and call wins_for_week / losses_for_week / bonus_for_week, each of which calls picks_for_week

update_awards genuinely does this (app/jobs/update_awards.py:27,74-75 → Player.wins_for_week/losses_for_week). But update_player_records does not: it calls player.recalculate_record(session) (app/jobs/update_player_records.py:10), and recalculate_record (app/models/player.py:99-113) iterates the game_picks relationship once and computes the record in a single pass — it never touches picks_for_week or the *_for_week helpers. So the "three queries per player per week" characterization and the app/jobs/update_player_records.py file reference are wrong for the current code; only update_awards is a beneficiary. Given the PR's stated premise ("Every file path and symbol referenced in the document is verified to exist"), this is the one entry that would mislead the next reader who acts on it. Either drop update_player_records from the entry or note it was migrated off the helpers.

Nits

docs/TECH_DEBT.md — "Routes and templates are largely untested" references app/routers/v2 as if it were a package. The actual file is app/routers/v2.py (a single module, not a directory). Minor, but the doc elsewhere is precise about .py vs. package, so worth normalizing.

Verified accurate (no action needed)

For the record, these claims all check out against the tree:

The new "A slow job can make the dispatcher's next tick fail" entry: the launch_* functions (app/jobs/scheduler.py:171-231) all call add_job with a fixed id and no replace_existing, while the schedule_* functions pass replace_existing=True — exactly as described, and add_games_for_current_week is a one-shot ADD_GAMES launcher.
picks_form swallows every IntegrityError: app/main.py:440-446 catches bare sqlalchemy.exc.IntegrityError, rolls back, and then renders picks_form.j2 (the success template).
PlayerGamePick docstring still documents season/week_no as denormalized :ivar: columns (app/models/player_game_pick.py:21-36) that no longer exist on the model.
All the "Removed" entries correspond to genuinely absent files (create_picks.py, award_update_all.py, sync_team_records.py, data_source.py, old tank01_api.py internals), and the "still uncovered" test list matches tests/ and app/routers/.
Version bump to 2.20.1 is applied consistently in both pyproject.toml and uv.lock, per the CLAUDE.md convention.

This is a documentation-only PR (plus the routine patch version bump), so I verified the debt entries against the actual code rather than looking for runtime bugs. The sweep is accurate almost everywhere I checked — but one carried-over entry names a job that no longer works the way the entry describes. --- ## Review A clean, well-organized sweep of `docs/TECH_DEBT.md` that removes entries for code that no longer exists and rewrites/adds three that reflect the current state. I spot-checked the file paths, symbols, and behavioral claims against the tree; the overwhelming majority hold, but the `picks_for_week` entry misattributes the fan-out cost to a job that has since stopped using that code path. ### Worth fixing - **`docs/TECH_DEBT.md` — the `picks_for_week lost its session-scoped cache` entry misdescribes `update_player_records`.** The entry claims: > The call sites that could justify it are the fan-out ones: `update_player_records` and `update_awards` both loop every active player and call `wins_for_week` / `losses_for_week` / `bonus_for_week`, each of which calls `picks_for_week` `update_awards` genuinely does this (`app/jobs/update_awards.py:27,74-75` → `Player.wins_for_week`/`losses_for_week`). But `update_player_records` does **not**: it calls `player.recalculate_record(session)` (`app/jobs/update_player_records.py:10`), and `recalculate_record` (`app/models/player.py:99-113`) iterates the `game_picks` relationship once and computes the record in a single pass — it never touches `picks_for_week` or the `*_for_week` helpers. So the "three queries per player per week" characterization and the `app/jobs/update_player_records.py` file reference are wrong for the current code; only `update_awards` is a beneficiary. Given the PR's stated premise ("Every file path and symbol referenced in the document is verified to exist"), this is the one entry that would mislead the next reader who acts on it. Either drop `update_player_records` from the entry or note it was migrated off the helpers. ### Nits - **`docs/TECH_DEBT.md` — "Routes and templates are largely untested" references `app/routers/v2` as if it were a package.** The actual file is `app/routers/v2.py` (a single module, not a directory). Minor, but the doc elsewhere is precise about `.py` vs. package, so worth normalizing. ### Verified accurate (no action needed) For the record, these claims all check out against the tree: - The new "A slow job can make the dispatcher's next tick fail" entry: the `launch_*` functions (`app/jobs/scheduler.py:171-231`) all call `add_job` with a fixed `id` and **no** `replace_existing`, while the `schedule_*` functions pass `replace_existing=True` — exactly as described, and `add_games_for_current_week` is a one-shot `ADD_GAMES` launcher. - `picks_form swallows every IntegrityError`: `app/main.py:440-446` catches bare `sqlalchemy.exc.IntegrityError`, rolls back, and then renders `picks_form.j2` (the success template). - `PlayerGamePick` docstring still documents `season`/`week_no` as denormalized `:ivar:` columns (`app/models/player_game_pick.py:21-36`) that no longer exist on the model. - All the "Removed" entries correspond to genuinely absent files (`create_picks.py`, `award_update_all.py`, `sync_team_records.py`, `data_source.py`, old `tank01_api.py` internals), and the "still uncovered" test list matches `tests/` and `app/routers/`. - Version bump to `2.20.1` is applied consistently in both `pyproject.toml` and `uv.lock`, per the CLAUDE.md convention.

johnsturgeon merged commit 25d0a7d996 into main

2026-09-01 18:55:02 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-01 18:55:02 +02:00

Merge pull request 'Sweep TECH_DEBT entries the control loop and #440 resolved' (#441) from sweep-tech-debt-resolved-by-the-control-loop-436 into main

johnsturgeon deleted branch sweep-tech-debt-resolved-by-the-control-loop-436

2026-09-01 18:55:08 +02:00