Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure triggering of callback in chord #397

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Ensure triggering of callback in chord #397

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e0aa80
Ensure triggering of callback in chord
s-bessing Jul 3, 2023
90344aa
Fixing line length
s-bessing Jul 3, 2023
5c6b5df
Merge branch 'celery:main' into patch-1
s-bessing Apr 30, 2024
1b04c88
update message
Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
update message 
fixing unit test
  • Loading branch information
Sebastian Jakob committed Apr 30, 2024
commit 1b04c887c57c7a12a85688cc4d0aeda378d89bee
4 changes: 2 additions & 2 deletions django_celery_results/backends/database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,8 @@ def on_chord_part_return(self, request, state, result, **kwargs):
)
chord_counter.delete()
else:
logger.warning("ChordCounter for Group %s 0 "
"but not ready yet.", gid)
logger.warning("Chord %s executed more "
"tasks than expected", gid)


def trigger_callback(app, callback, group_result):
Expand Down
4 changes: 2 additions & 2 deletions t/unit/backends/test_database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,8 +862,8 @@ def test_on_chord_part_return_failure(self):
request.id = tid2
self.b.mark_as_failure(tid2, ValueError(), request=request)

with pytest.raises(ChordCounter.DoesNotExist):
ChordCounter.objects.get(group_id=gid)
chord_counter.refresh_from_db()
assert chord_counter.count == 1
Comment on lines +865 to +866
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct, we have a header of 2, 1 is done, the other fails.
If a task in the header fails the chord is done and the callback is not called.

If this is the case, a ChordCounter object will remain in the db for every failed chord.

Copy link
Author

Choose a reason for hiding this comment

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

@AllexVeldman but if you have retry logic (on code or worker level) all task eventually are successful, but the callback is never triggered.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://docs.celeryq.dev/en/latest/reference/celery.result.html#celery.result.AsyncResult.ready would suggest deps.ready() would be False in case of a retry, True in case of a failure like in this test.

So this test should call the path all the way to trigger_callback() and fail the Chord, otherwise a Chord can never fail.
It suprises me that this is not the case with your proposed changes, since trigger_callback should be True, deps.ready() should be True so both trigger_callback() and chord_counter.delete() should have been called.

Copy link
Contributor

@AllexVeldman AllexVeldman Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

OK, it took some time but I figured out why this change makes the test pass (which is still not correct).

It's because the MagicMock used for the request parameter to mark_as_done() and mark_as_failure() does not have ignore_result = False set. This in turn would make request.ignore_result evaluate to True, skipping saving the result, which is needed to have deps.ready() work properly. Since we did not rely on deps.ready() in the previous state this never surfaced as an issue.

Add ignore_result = False to the MagicMock and reverting the test changes will fix the test.


request.chord.delay.assert_not_called()

Expand Down
Loading