Skip to content

Commit

Permalink
Fix an over-eager removal of a Celery task arg
Browse files Browse the repository at this point in the history
In a previous commit, I removed the deprecated `lottery_config` argument
from `run_lottery`, thinking I could safely just deprecate old messages
by capturing `lottery_config` with `**kwargs`.

Looking at this code, I now realize that `lottery_config` is being
passed conditionally.

To safely keep the lottery running on all current trips I need to:

1. Deploy this code
2. Restart Celery workers
3. Revoke all open messages identified by `trip.lottery_task_id`

Step 3 can be accomplished by just editing all open lottery trips,
making the close time marginally different.
  • Loading branch information
DavidCain committed Jul 15, 2022
1 parent 871816a commit d43ed1a
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 10 deletions.
5 changes: 2 additions & 3 deletions ws/signals/signup_signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ def revoke_existing_task(sender, instance, raw, using, update_fields, **kwargs):
except sender.DoesNotExist: # New trip, initiated with no task
return

# TODO: There's a race condition here; we should lock `trip` exclusively
new_close_time = instance.signups_close_at != trip.signups_close_at
needs_revoke = new_close_time or trip.algorithm != 'lottery'
if trip.lottery_task_id and needs_revoke:
Expand Down Expand Up @@ -132,9 +133,7 @@ def add_lottery_task(sender, instance, created, raw, using, update_fields, **kwa
return # Only new lottery trips get a new task

try:
task_id = tasks.run_lottery.apply_async(
(trip.pk, None), eta=trip.signups_close_at
)
task_id = tasks.run_lottery.apply_async((trip.pk,), eta=trip.signups_close_at)
except OperationalError:
logger.error("Failed to make lottery task for trip %s", trip.pk)
else:
Expand Down
8 changes: 1 addition & 7 deletions ws/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,7 @@ def purge_old_medical_data():


@mutex_task('single_trip_lottery-{trip_id}')
def run_lottery(
trip_id: int,
# NOTE: Can delete `kwargs` later.
# This exists to handle messages which are already queued,
# and may include the unused `lottery_config` argument
**_kwargs,
):
def run_lottery(trip_id: int):
"""Run a lottery algorithm for the given trip (idempotent).
If running on a trip that isn't in lottery mode, this won't make
Expand Down

0 comments on commit d43ed1a

Please sign in to comment.