Skip to content

Conversation

@khvn26
Copy link
Member

@khvn26 khvn26 commented May 28, 2025

This PR adds support for task backoff via raising a TaskBackoffError.

The task runner now reschedules a task if the above exception class is raised inside a regular (non-recurring) task. The exception class supports a delay_until argument to specify which datetime the next task invocation should be scheduled for. If not specified, settings.TASK_BACKOFF_DEFAULT_DELAY_SECONDS will be used.

@khvn26 khvn26 requested a review from a team as a code owner May 28, 2025 13:17
@khvn26 khvn26 requested review from Zaimwa9 and removed request for a team May 28, 2025 13:17
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2025

Codecov Report

Attention: Patch coverage is 98.70130% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.42%. Comparing base (b6a250b) to head (af18bca).

Files with missing lines Patch % Lines
...sk_processor/test_unit_task_processor_processor.py 97.77% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (98.70%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   94.17%   94.42%   +0.25%     
==========================================
  Files          74       74              
  Lines        2147     2208      +61     
==========================================
+ Hits         2022     2085      +63     
+ Misses        125      123       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My general concern with this is that we could end up with a task infinitely failing and being rescheduled, right? Also, I don't see any logic here that 'nullifies' the original retry logic that will still retry the task as soon as possible, to a maximum of 3 times. Shouldn't we also intercept that logic somehow?

Comment on lines 167 to 169
assert registered_task.task_handler, (
"Attempt to back off a recurring task (currently not supported)"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use registered_task.task_type here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion also reassures Mypy that the task_handler attribute is not None.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified/cleaned up in b289839.

@khvn26
Copy link
Member Author

khvn26 commented May 28, 2025

@matthewelwell

we could end up with a task infinitely failing and being rescheduled, right?

This is a valid concern. We could do one of the following to address it:

  1. Add a num_failures argument to the .delay method to carry over the number of failures when scheduling the task.
  2. Do not use delay at all to re-create the task and modify the scheduled_for field in-place for the existing one.

Note that both of the options will cap the backoff attempts at the currently hard-coded value of 3 attempts.

Shouldn't we also intercept that [retry] logic somehow?

Good catch, done in b905d54 — but that will need to be reverted should we choose the option (2) above.

@khvn26 khvn26 requested review from emyller and matthewelwell May 28, 2025 16:13
@khvn26 khvn26 force-pushed the feat/task-backoff branch from b905d54 to 9de8df5 Compare May 28, 2025 16:43
@khvn26
Copy link
Member Author

khvn26 commented May 28, 2025

Implemented option 2 in 9de8df5, which also resulted in a leaner overall diff 👍

@khvn26 khvn26 merged commit a736a68 into main May 30, 2025
4 checks passed
This was referenced Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants