Skip to content

Conversation

simonepozzoli-pix4d
Copy link
Contributor

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

Preserve group ordering when replacing a task with a group.
It should not be allowed to pass a group_index in the options, otherwise all tasks in the group will have the same group_index and the ordering is not deterministic.

Related issue: #9909

Copy link

codecov bot commented Sep 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.67%. Comparing base (8063230) to head (509d30b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9910   +/-   ##
=======================================
  Coverage   78.67%   78.67%           
=======================================
  Files         153      153           
  Lines       19299    19299           
  Branches     2570     2570           
=======================================
  Hits        15184    15184           
  Misses       3816     3816           
  Partials      299      299           
Flag Coverage Δ
unittests 78.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you please add unit tests to verify the change?

@simonepozzoli-pix4d
Copy link
Contributor Author

can you please add unit tests to verify the change?

Added a test here fac9593
However, I had to set eager execution of the task to make it work. This is not a good representation of what is happening in a live environment where the order of execution of the tasks is not preserved.
The issue I'm trying to fix in this PR happens only if a worker with concurrency > 1 registers the result of the task in the group's second place before registering the result of first task in the group. Since both results have the same group_index, the ordering depends only on which task finished first.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where group ordering was not preserved when replacing a task with a group. The fix prevents group_index from being passed in options to avoid all tasks in the group having the same index, which would make ordering non-deterministic.

  • Adds group_index parameter to _apply_tasks method to handle group indexing explicitly
  • Includes a test case to verify group order preservation when task replacement occurs

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
celery/canvas.py Adds group_index parameter to _apply_tasks method signature to handle group indexing
t/unit/tasks/test_canvas.py Adds test case to verify group order is preserved during task replacement

@simonepozzoli-pix4d
Copy link
Contributor Author

simonepozzoli-pix4d commented Sep 30, 2025

I added a second integration test in 1343410
Without my change, the test is consistently failing.
Here some examples. As I mentioned, the result order is not deterministic:

======================================== short test summary info ========================================
FAILED t/integration/test_canvas.py::test_group::test_task_replace_with_group_preserves_group_order - assert [[2, 3], [5, ..., [5, 4], ...] == [[3, 2], [5, ..., [5, 4], ...]
============================================================================================= short test summary info =============================================================================================
FAILED t/integration/test_canvas.py::test_group::test_task_replace_with_group_preserves_group_order - assert [[3, 2], [4, ..., [4, 5], ...] == [[3, 2], [5, ..., [5, 4], ...]
============================================================================================= short test summary info =============================================================================================
FAILED t/integration/test_canvas.py::test_group::test_task_replace_with_group_preserves_group_order - assert [[3, 2], [4, ..., [5, 4], ...] == [[3, 2], [5, ..., [5, 4], ...]

@simonepozzoli-pix4d
Copy link
Contributor Author

can you please add unit tests to verify the change?

Added a test here fac9593 However, I had to set eager execution of the task to make it work. This is not a good representation of what is happening in a live environment where the order of execution of the tasks is not preserved. The issue I'm trying to fix in this PR happens only if a worker with concurrency > 1 registers the result of the task in the group's second place before registering the result of first task in the group. Since both results have the same group_index, the ordering depends only on which task finished first.

Just a small clarification. I said the result ordering depends on the timing of tasks execution. This is not always correct.
We are using Redis as a result backend. When replacing a task with a group, the current task is replaced by a chord with header equal to the replacing tasks and body equal to celery.accumulate task. In Redis the chord header results are stored in a sorted set and forwarded to the chord body when all of them are available. If all results in the sorted set have the same score, they are sorted alphabetically when fetched from Redis using the ZRANGE command (https://redis.io/docs/latest/commands/zrange/#common-behavior-and-options).

simonepozzoli-pix4d and others added 2 commits October 1, 2025 09:44
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@auvipy auvipy requested a review from Copilot October 1, 2025 09:47
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

This reverts commit 39385e0.
The orignal commit introduced a syntax error.
@auvipy auvipy merged commit 88f3cc7 into celery:main Oct 4, 2025
109 checks passed
@auvipy auvipy added this to the 5.6.0 milestone Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants