celery: wait for exported spans instead of result.ready() in tests - #5015
Open
aniketwaghh wants to merge 1 commit into
Open
celery: wait for exported spans instead of result.ready() in tests#5015aniketwaghh wants to merge 1 commit into
aniketwaghh wants to merge 1 commit into
Conversation
Celery stores the task result before it dispatches task_postrun, and CeleryInstrumentor ends the run span from its task_postrun receiver. So an AsyncResult can be ready while the run span has not been ended and exported yet, and the span assertions run against an incomplete exporter. Wait for the expected number of exported spans instead. The window is a fraction of a millisecond on an idle machine and unbounded on a loaded CI runner, which is what makes test_task_raises fail with 1 != 2. test_task_clears_start_time_cache also waits for the start-time cache to drain, since that pop happens a few statements after the span is ended in the same receiver. test_baggage and test_uninstrument keep result.ready(), which is the right thing for them to wait on.
|
|
Pull request dashboard statusWaiting on reviewers · refreshed 2026-08-30 07:23 UTC Review the latest changes. Status above doesn't look right?
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The celery tests wait on
result.ready()before asserting on spans, but Celery stores the task result before it dispatchestask_postrun, andCeleryInstrumentorends the run span from itstask_postrunreceiver. So the result can be ready while the run span has not been ended and exported yet, and the assertion runs against an incomplete exporter. That is the1 != 2in the issue.These now wait for the expected number of exported spans instead.
test_task_clears_start_time_cachealso waits for the start-time cache to drain, because that pop happens a few statements after the span is ended in the same receiver.test_baggageandtest_uninstrumentkeepresult.ready(), which is the right thing for them to wait on.Test-only change, no instrumentation code touched.
Two things worth flagging:
breakon timeout and fall through into the assertion, so a genuine hang showed up as a confusing span-count mismatch. These raise with the reason instead.test_metrics.pywaits the same way and has the same race. I left it alone to keep this to one logical change, but it is the same fix if you want it. It already carries askipif(PyPy, reason="Fails randomly in pypy"), which looks like this race showing up there.Prior analysis of this race in the docker suite is in #4964 by @Pissinatti-py, for #653. Different file and different issue, so no overlap with this change, but they described the cause first and it is worth reading alongside.
Fixes #5009
Type of change
How Has This Been Tested?
Ordering measured on macOS arm64 against
f1b9368aa, showing the result landing before the span ends:To make it deterministic rather than waiting for a loaded runner, I connected a 0.5s
task_postrunreceiver ahead of the instrumentor's, which widens the real window without touching any source:With that loaded, on
main:test_task_raisesfails withAssertionError: 1 != 2attest_tasks.py:123, the error and line from the CI traceback in the issue.With this change, same widened window:
The six in
test_tasks.pyare gone. The two remaining aretest_metrics.py, which this PR does not touch.Unmodified suite:
mainand confirmed the fix removes itDoes This PR Require a Core Repo Change?
Checklist:
Skip Changeloglabel rather than a fragment. Happy to add one if you would rather have it.