Don't fail a task whose SUCCESS was stored before the worker was lost - #10752

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.72%. Comparing base (380b4a9) to head (aa4e0b2).

@@           Coverage Diff           @@
##             main   #10752   +/-   ##
=======================================
  Coverage   90.72%   90.72%           
=======================================
  Files         153      153           
  Lines       21549    21571   +22     
  Branches     2653     2657    +4     
=======================================
+ Hits        19551    19571   +20     
- Misses       1685     1687    +2     
  Partials      313      313           

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

@auvipy the change is covered by unit tests in t/unit/worker/test_request.py:

• test_on_failure_WorkerLostError_after_success_was_stored — SUCCESS stays SUCCESS, errbacks do not run
• test_on_failure_WorkerLostError_after_success_was_stored_acks_late — acknowledges instead of rejecting/requeueing when reject_on_worker_lost is set
• test_on_failure_WorkerLostError_backend_unavailable — falls back to the old failure path if the backend cannot be read

There is no separate smoke/integration case for this; reproducing it end-to-end needs a child that stores SUCCESS and then dies before the parent collects the result. I just merged current main and kept the SUCCESS guard ahead of the new retry_on_worker_lost path so a finished task is not retried.

@ChristianMurphy can you review this? may be merging this before your one would be good idea

I tested this against db0da7c on Redis, SQLite and rpc://, with a prefork worker and a task_success receiver that kills the child with SIGKILL after SUCCESS is stored.

On Redis and SQLite it works as described. The errback and task_failure don't fire, and a chord with that task in its header runs its body instead of failing with ChordError. With acks_late and reject_on_worker_lost the task runs once, where main requeues it (62 runs in 8 seconds when the crash repeats). retry_on_worker_lost doesn't send it again either.

On merge order: this merges cleanly with #10920 and #10759. Each of the three fixes a different path, and any order works.

Some things to consider:

1. The worker doesn't send a final event. Main sends task-failed. With this change the last event is task-started, so events.State (and Flower) show the task as STARTED forever. Could it send task-succeeded?
2. On rpc:// the lookup always returns PENDING, because results go to the caller's queue, so the behavior doesn't change. But each worker process now declares a durable queue named after its oid, which stays for 24 hours. Skipping the lookup for rpc://, and for ignore_result tasks, would avoid that.
3. A SUCCESS from an earlier delivery of the same task id also matches. On Redis, when a task id is sent again and its second run is killed mid-task, the message is acked instead of requeued, and the warning says the task succeeded.
4. A test for retry_on_worker_lost would help.

Some things to consider:

1. The worker doesn't send a final event. Main sends task-failed. With this change the last event is task-started, so events.State (and Flower) show the task as STARTED forever. Could it send task-succeeded?
2. On rpc:// the lookup always returns PENDING, because results go to the caller's queue, so the behavior doesn't change. But each worker process now declares a durable queue named after its oid, which stays for 24 hours. Skipping the lookup for rpc://, and for ignore_result tasks, would avoid that.
3. A SUCCESS from an earlier delivery of the same task id also matches. On Redis, when a task id is sent again and its second run is killed mid-task, the message is acked instead of requeued, and the warning says the task succeeded.
4. A test for retry_on_worker_lost would help.

@gyanu2507 please check

Addressed @ChristianMurphy's review notes:

1. When SUCCESS was already stored, the worker now sends task-succeeded (with the stored result) so Flower / events.State do not stay on STARTED.
2. _success_was_stored skips the lookup for ignore_result tasks and for the rpc:// backend, so we do not declare a durable per-worker reply queue for a check that would always see PENDING.
3. A SUCCESS from an earlier delivery of the same task id is still a known limitation of a pure get_state check. Happy to follow up if maintainers want a stronger correlation (for example tying the stored result to the current delivery).
4. Added a unit test that retry_on_worker_lost does not re-send when SUCCESS is already stored, plus coverage for the rpc:// and ignore_result skip paths.

Also restored the missing ======= underline under the 5.7.0b1 changelog heading if that had been dropped by a main merge.

Suggestions:

* Skip the lookup when `self.task.ignore_result` is set. Together with skipping `rpc://` (point 2), that covers the cases where there's nothing to find.

* Make it a single attempt that doesn't go through the retry loop, e.g. read `backend._get_task_meta_for(self.id)` inside the existing `try`, so a backend that's down falls back right away as the comment says.

* Add a test with `result_backend_always_retry=True` and a backend whose read fails, checking that `on_failure` falls back without sleeping.

@gyanu2507 are this also covered?

I tested 0d81e01 again with a prefork worker on Redis, killing the child right after SUCCESS is stored. The main fix works.

I'd change two more things.

1. Put a saferepr of the result in the event, as a normal success does.

The new task-succeeded event (request.py:720) sends the whole result. A normal success sends saferepr(result, resultrepr_maxsize), which is capped at about 1,000 characters. Sending the whole result causes two problems:

• A task that returned 100,000 characters put all of them in the event.
• If the result can't be JSON encoded, the event isn't sent. I saw this with result_serializer='pickle' and an object as the result. The dispatcher puts the failed event in its buffer without logging anything, so Flower still shows STARTED.

Sending saferepr(result, self.task.resultrepr_maxsize) fixes both. No test checks the payload yet. One would catch this.

2. Check the request's ignore_result, not the task's.

_success_was_stored checks task.ignore_result (request.py:831). The request has its own self.ignore_result, which also reads the ignore_result option passed to apply_async(). Checking the task's value misses two cases:

• apply_async(ignore_result=True) still does the lookup. With result_backend_always_retry=True and the backend down, on_failure never returns. That's the hang MehrazRumman found, still reachable this way.
• With task_ignore_result=True in the settings and apply_async(ignore_result=False), SUCCESS is stored, but the lookup is skipped. The task then fails the same way it does on main.

Using self.ignore_result fixes both. test_on_failure_WorkerLostError_skips_lookup_when_ignore_result would then need to set the option on the request instead of the task.

Addressed the follow-ups from @ChristianMurphy:

1. The late task-succeeded event now sends saferepr(result, self.task.resultrepr_maxsize) instead of the raw result, matching a normal success path for size and JSON-safety. The acks_late unit test asserts the payload is the saferepr string.
2. _success_was_stored now checks self.ignore_result (request-level, including apply_async(ignore_result=...)) instead of only task.ignore_result. The ignore_result unit test sets the flag on the request.

Also merged current main.