Skip to content

refactor: simplify run_recurring_tasks for single-task processing#218

Open
gagantrivedi wants to merge 3 commits intomainfrom
refactor/simplify-run-recurring-tasks
Open

refactor: simplify run_recurring_tasks for single-task processing#218
gagantrivedi wants to merge 3 commits intomainfrom
refactor/simplify-run-recurring-tasks

Conversation

@gagantrivedi
Copy link
Copy Markdown
Member

@gagantrivedi gagantrivedi commented Apr 28, 2026

  • I have read the Contributing Guide.
  • I have added information to docs/ if required so people know about the feature.
  • I have filled in the "Changes" section below.
  • I have filled in the "How did you test this code" section below.

Changes

Contributes to #152

The SQL function backing RecurringTaskManager (get_recurringtasks_to_process) is LIMIT 1, so it always returns 0 or 1 rows. Despite that, run_recurring_tasks was structured around a loop with bulk_create for RecurringTaskRuns and bulk_update for the RecurringTask lock columns — bulk machinery operating on at most one row.

This PR:

  • Replaces the loop + bulk pipeline in run_recurring_tasks with straight-line single-task handling: pick the one row, dispatch on is_task_registered / should_execute, then task.save(update_fields=…) and task_run.save() directly.
  • Renames RecurringTaskManager.get_tasks_to_process()get_task_to_process() and changes its return type from RawQuerySet[RecurringTask] to RecurringTask | None, so the Python contract matches the SQL.
  • Leaves the underlying SQL function name (get_recurringtasks_to_process) untouched — it's referenced from migration history (reverse_sql) and renaming it would force a migration for zero functional gain.
  • Preserves all existing comments verbatim and the original if task.should_execute / else task.unlock() control flow.

TaskManager.get_tasks_to_process(num_tasks) (for non-recurring Tasks) is genuinely multi-row and is unchanged.

How did you test this code?

  • Covered by existing unit tests

The underlying SQL function `get_recurringtasks_to_process` is `LIMIT 1`,
so the loop, bulk_create, and bulk_update were always operating on at
most one row. Replace the bulk pipeline with straight-line single-task
handling and rename the manager method to `get_task_to_process`,
returning `RecurringTask | None`, so the Python contract matches the
SQL.
@gagantrivedi gagantrivedi requested a review from a team as a code owner April 28, 2026 10:52
@gagantrivedi gagantrivedi requested review from emyller and removed request for a team April 28, 2026 10:52
Declare `task_run` as `RecurringTaskRun | None` up front and route the
`_run_task` return through an intermediate `run` so the `assert
isinstance` narrowing carries past the `if/else` join. Also include the
task identifier in the start/finish debug logs for easier tracing.
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.28%. Comparing base (273d3ad) to head (072e69b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
+ Coverage   97.26%   97.28%   +0.02%     
==========================================
  Files         104      104              
  Lines        4459     4464       +5     
==========================================
+ Hits         4337     4343       +6     
+ Misses        122      121       -1     

☔ 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.

The function only ever processes one recurring task per call (the
underlying SQL is `LIMIT 1`), so a list was always 0 or 1 items long.
Tighten the signature to `RecurringTaskRun | None` so the contract
matches reality and mirrors `RecurringTaskManager.get_task_to_process()`.

The sole production caller (`threads.run_iteration`) discards the return
value and is unaffected. All recurring test sites are updated to use the
singular value: `len(...) == 1` becomes `is not None`, list indexing is
removed, and the empty-result case becomes `is None`. A
`test_run_recurring_tasks__no_tasks__does_nothing` test is added,
mirroring the existing standard-task equivalent, to cover the
empty-queue early return.
Copy link
Copy Markdown
Contributor

@emyller emyller left a comment

Choose a reason for hiding this comment

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

This looks like it also limits overall recurring tasks to one per iteration, despite the caller is still named run_recurring_tasks.

In practice, we'd be running pending recurring tasks one after another, with a one second delay:

def run(self) -> None:
while not self._stopped:
self.last_checked_for_tasks = timezone.now()
self.run_iteration()
time.sleep(self.sleep_interval_millis / 1000)

Please update the logic so multiple recurring tasks can run per iteration, or make the intended behaviour explicit — e.g. in function names, comments, etc.

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.

3 participants