From d43ed1a1f30c4e8f3579a8e2c4ac306b5a49eb42 Mon Sep 17 00:00:00 2001 From: David Cain Date: Thu, 14 Jul 2022 21:25:58 -0700 Subject: [PATCH] Fix an over-eager removal of a Celery task arg 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. --- ws/signals/signup_signals.py | 5 ++--- ws/tasks.py | 8 +------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/ws/signals/signup_signals.py b/ws/signals/signup_signals.py index 8edb2125..a304a1e6 100644 --- a/ws/signals/signup_signals.py +++ b/ws/signals/signup_signals.py @@ -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: @@ -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: diff --git a/ws/tasks.py b/ws/tasks.py index 36a40643..1c291f47 100644 --- a/ws/tasks.py +++ b/ws/tasks.py @@ -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