Arm deadline warnings per phase and delete the per-minute sweep - #1391
Merged
Merged
Conversation
The phase.send_deadline_warnings cron ran every minute over every active game with a heavily prefetched queryset. Replace it with a phase.send_deadline_warning job armed at scheduled_resolution minus the warning offset, mirroring the resolution arming. The phase post_save receiver and the NMR extension path re-arm it when the deadline or status changes. A job that already fired for an unchanged deadline is left alone, so a same-deadline save does not warn twice. Recipients are evaluated when the job fires, so the per-player deadline_warning_sent_for dedup check goes away. The field stays: the fire-time method still writes it and reads it to choose the "deadline has been extended" copy from #1364. The warning-threshold table moves into phase.utils.deadline_warning_offset. Migration 0023 adds Phase.warning_job_id and arms a warning for every live phase whose warning time is still ahead, so phases already running at deploy keep their warning. Closes #1073. Supersedes #1097. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRFZ6aT4agJnpbA6WXJJVc
Contributor
|
Warning WIP limit exceeded. There are now 8 open pull requests — the project target is 5 or fewer. Please close or merge an existing PR before continuing with this one. |
Unpausing saved the phase while the game was still paused, so arm_warning cancelled the job and nothing re-armed it. pause and unpause now re-arm the warning alongside the resolution. A warning job already running when the deadline moved could not be cancelled and sent early against the new deadline. The fire-time method now also requires the warning time for the current deadline to have arrived. arm_warning kept any job whose schedule matched, including failed and cancelled ones. It now keeps only todo, doing and succeeded jobs. The send now runs in one transaction with the phase row locked, and it again skips a player already warned for this exact deadline. A retry, a duplicate run or a racing job cannot warn anyone twice, and a failed send leaves no notifications or markers behind. The backfill migration arms every live phase, including those already inside their warning window. The per-deadline marker keeps players the old sweep already warned from being warned again, which closes the gap between the last sweep and the migration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRFZ6aT4agJnpbA6WXJJVc
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Closes #1073 and supersedes the stale #1097. This is a fresh reimplementation on current
main.It deletes the
phase.send_deadline_warningscron job, which ran every minute and scanned every active game with a heavily prefetched queryset. In its place, each phase gets its ownphase.send_deadline_warning(phase_id)job, scheduled atscheduled_resolution - deadline_warning_offset(...). This works the same way phase resolution is already scheduled.Scheduling the job
Phase.warning_job_idandPhaseManager.arm_warning.arm_warninghas the same structure asarm_resolution: it reads the current job, computes the schedule, and if the schedule changed it cancels the old job and defers a new one. It is called from:post_savereceiver, next to the resolution one;_apply_nmr_extensions, because that path moves the deadline with.update(), which does not firepost_save;Game.pause/Game.unpause.unpausesaves the phase while the game is still paused, so without this the warning would be cancelled and never re-armed.scheduled_atstill matches and its status istodo,doingorsucceeded. So a warning that has already gone out is not armed again when the phase is saved with the same deadline. Afailedorcancelledjob is replaced.Sending the warning
send_deadline_warningruns in one transaction with the phase row locked. It only sends when all of these hold:filter_armable());It keeps the old per-player checks (
has_possible_orders, forced orders, actionable units, not confirmed, has a user). It also skips any player already warned for this exact deadline, usingdeadline_warning_sent_for. Because of that check and the transaction, a retry, a duplicate run or two jobs racing cannot warn anyone twice, and a failed send leaves no notifications or markers behind.It emits
deadline_warningexactly as before; the notification copy is unchanged.Other changes
phase.utils.deadline_warning_offset.send_deadline_warningsmanager method.deadline_warning_sent_foris kept (differs from #1097)#1364 merged after #1097 and gave this field a second job: when a player was warned at an earlier deadline on the same phase, their next warning starts with "The deadline has been extended - still …". So the field stays. It now does two things:
Backfill in the migration (beyond #1097)
Migration
0023adds the field and then arms a warning job for every live phase, following0021_arm_resolution_jobs. Phases already inside their warning window are armed too, so their job runs immediately. The per-deadline marker skips players the old sweep already warned, and there is no gap between the last sweep and the migration.Tests
test_background_resolution.py::TestDeadlineWarningArmingcovers:deadline - offset;extend_deadlineor NMR extension;test_deadline_reminders.pycovers the fire-time checks:phase/tests.py: the body-copy tests now callsend_deadline_warning(phase.id), including both Stop deadline warnings piling up and repeating themselves #1364 "extended" tests.test_utils.py: a parametrised test fordeadline_warning_offset.phaseandgamewent up by 1–5 queries per phase save, all from arming the warning. I checked this against the logged queries.Full backend suite: 2556 passed, 8 skipped, 1 failed. The failure is
harness/tests.py::TestQualityMetricAggregation::test_accuracy_covers_ranked_samples_and_skips_the_rest, which also fails onmainwithout this change.Checklist
/review-pragainst this PR in Claude Code and addressed (or responded to) its findingsNote: this makes 8 open PRs, over the soft limit of 5.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TRFZ6aT4agJnpbA6WXJJVc