fix: Preserve call/errbacks of replaced tasks

[maybe-sybr]

Description

This change fixes the handling of callbacks and errbacks on tasks which get replaced to
ensure they get called as expected if the replacement signature succeeds or fails.

Fixes #6441

[maybe-sybr]

This is a bugfix but I think we can land it on master once we sort out 5.1 since it's been broken for a while. :) It'll improve my life but FWICT there are no issues from other users tracking this misbehaviour.

[lgtm-com]

This pull request introduces 1 alert when merging d8bf93f into 2411504 - view on LGTM.com

new alerts:

• 1 for Non-exception in 'except' clause

[thedrow]

I'll review the integration tests later on.
Please check the build log as we have a genuine test failure, I believe.

[maybe-sybr]

t/unit/tasks/test_tasks.py::test_tasks::test_replace_callback() is failing because I'm linking the callbacks and errbacks of the replaced signature to a replacement group even though they should should only be linked to the accumulate task that we chain onto the end of the group signature. Since a callback/errback was being set for a group, when that group was eventually applied it would die due to the TypeError() we were commenting on above:

self = group([t.unit.tasks.test_tasks.return_True()]), args = (), kwargs = None, add_to_parent = True, producer = None
link = ['callbacks'], link_error = ['errbacks']
options = {'group_id': 'group', 'group_index': None, 'root_id': 'root_id', 'task_id': '739d4d46-a741-4db5-aa5b-eacdb75de6bf'}

    def apply_async(self, args=None, kwargs=None, add_to_parent=True,
                    producer=None, link=None, link_error=None, **options):
        args = args if args else ()
        if link is not None or link_error is not None:
>           raise TypeError('Cannot add linked tasks to groups: use a chord')
E           TypeError: Cannot add linked tasks to groups: use a chord

celery/canvas.py:1076: TypeError

I've changed the logic in replace() to only link call/errbacks to the replacement signature if it's not a group. I'll push a new diff once I've done some more sanity checking on the integration tests.

[codecov]

Codecov Report

Merging #6770 (5370fdb) into master (536849c) will increase coverage by 0.01%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##           master    #6770      +/-   ##
==========================================
+ Coverage   70.71%   70.73%   +0.01%     
==========================================
  Files         138      138              
  Lines       16604    16605       +1     
  Branches     2094     2094              
==========================================
+ Hits        11741    11745       +4     
+ Misses       4667     4663       -4     
- Partials      196      197       +1     

Flag	Coverage Δ
unittests	70.73% <53.33%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
celery/canvas.py	94.82% <ø> (ø)
celery/app/task.py	94.65% <53.33%> (+0.81%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 536849c...5370fdb. Read the comment docs.

[lgtm-com]

This pull request introduces 2 alerts and fixes 1 when merging 00d5516 into 2411504 - view on LGTM.com

new alerts:

• 2 for Unused import

fixed alerts:

• 1 for Module is imported with 'import' and 'import from'

[lgtm-com]

This pull request introduces 1 alert when merging ff9f31f into 2411504 - view on LGTM.com

new alerts:

• 1 for Unused import

[maybe-sybr]

I've reworked the replacement logic entirely now. It could do with a re-review. AFAICT, removing the early replacement_sig.freeze(self.request.id) hasn't disrupted any of our unit or integration tests (against redis) so I'm reasonably confident that the diff is fine to merge.

[maybe-sybr]

This now behaves as I expect and we have some new integration tests to catch regressions or misbehaviours. That actually exposed an issue with chord header result ordering which I've also fixed in the commit on the tip of this branch.

[lgtm-com]

This pull request introduces 1 alert when merging d705a73 into 2411504 - view on LGTM.com

new alerts:

• 1 for Non-exception in 'except' clause

[lgtm-com]

This pull request introduces 1 alert and fixes 2 when merging 1095f05 into 2411504 - view on LGTM.com

new alerts:

• 1 for Unused import

fixed alerts:

• 1 for Non-exception in 'except' clause
• 1 for Module is imported with 'import' and 'import from'

[maybe-sybr]

That last fixup broke a unit test, it (should be) fixed again now.

[thedrow]

LGTM.

This code is much cleaner than before.

[maybe-sybr]

LGTM.

This code is much cleaner than before.

Great thanks :) I've just marked this for 5.2 so we can come back and merge it once we drop 5.1

[maybe-sybr]

I'll mark this as needs rebase to remind me to rebase on top of 5.1 as prep for merge

[lgtm-com]

This pull request introduces 1 alert when merging fcfbe43 into 025bad6 - view on LGTM.com

new alerts:

• 1 for Unused import

[auvipy]

rebase please. as it's a bug fix we can land this on 5.1.x as well

[auvipy]

rebase please. as it's a bug fix we can land this on 5.1.x as well

[maybe-sybr]

rebase please. as it's a bug fix we can land this on 5.1.x as well

rebased on master

[lgtm-com]

This pull request introduces 2 alerts when merging 5370fdb into 536849c - view on LGTM.com

new alerts:

• 1 for Non-exception in 'except' clause
• 1 for Module is imported with 'import' and 'import from'