fix: prevent redelivered run_bot tasks from re-running finished bots (#587) - #898
Open
yangtheman wants to merge 1 commit into
Open
fix: prevent redelivered run_bot tasks from re-running finished bots (#587)#898yangtheman wants to merge 1 commit into
yangtheman wants to merge 1 commit into
Conversation
…ttendee-labs#587) With CELERY_TASK_ACKS_LATE, the run_bot message stays unacked for the whole meeting. It gets redelivered when a worker is lost mid-task (reject_on_worker_lost) or when the Redis visibility_timeout (default 3600s) expires before the meeting ends. The redundant run then created an empty recording file that overwrote the completed recording. - run_bot now aborts immediately if the bot is already in a post-meeting state (FATAL_ERROR, ENDED, DATA_DELETED). - Default the Redis broker visibility_timeout to 21600s (env-overridable via CELERY_BROKER_VISIBILITY_TIMEOUT_SECONDS) so it exceeds the longest bot runtime; merged with CELERY_BROKER_TRANSPORT_OPTIONS.
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.
Summary
Follow-up to #897, addressing the trigger behind #587: why a
run_bottask runs a second time for an already-ENDED bot.Root cause
The production settings profiles set
CELERY_TASK_ACKS_LATE = TruewithCELERY_TASK_REJECT_ON_WORKER_LOST = True. Sincerun_botruns for the entire meeting, its message stays unacked the whole time and gets redelivered in two scenarios:run_botfor a bot that has meanwhile reachedENDED. This matches the reporter's "restart → recording wiped" sequence.visibility_timeoutexpiry — the Redis broker's default is 3600s and was not overridden, so any bot running longer than ~1 hour gets a duplicate, concurrentrun_botdelivery even without a restart.Nothing made
run_botidempotent, so the redundant run created aBotControllerthat never recorded and, on shutdown, uploaded an empty file over the completed recording (guarded separately in #897).Changes
run_botaborts immediately if the bot is already in a post-meeting state (BotStates.post_meeting_states()), as suggested in the issue discussion.visibility_timeoutto 21600s, env-overridable viaCELERY_BROKER_VISIBILITY_TIMEOUT_SECONDS, merged with the existingCELERY_BROKER_TRANSPORT_OPTIONSenv support (env keys win).The state guard handles redelivery for finished bots; the visibility_timeout bump prevents duplicate concurrent deliveries for still-running bots.
Trade-off: crash-recovery latency for short tasks
visibility_timeoutis broker-wide, so raising it to 6h also delays the redelivery safety net for short tasks (deliver_webhook,process_utterance, etc.): if a worker dies hard (OOM-kill/node loss, no graceful shutdown), its unacked messages now wait up to 6h instead of 1h before another worker restores them. In practice the blast radius is small:reject_on_worker_lost=Truerequeues immediately in the common child-process-death case (visibility_timeout not involved), andprefetch_multiplier=1means a dead worker holds at most ~1 extra message. All ETA/countdown usage in the codebase is ≤64s, far below either value — raising the timeout actually moves further from the classic Redis+Celery duplicate-ETA-execution failure mode. Operators who prefer the old behavior can setCELERY_BROKER_VISIBILITY_TIMEOUT_SECONDS=3600.Testing
bots/tests/test_run_bot_task.py— skips all post-meeting states, runs otherwise; passing.broker_transport_optionsresolves correctly with and withoutCELERY_BROKER_TRANSPORT_OPTIONSset.test_launch_scheduled_bot_task+test_throttlingstill passing.ruff checkandruff format --checkclean.Docs
No API change — no
docs/openapi.ymlupdates needed.