Don't record FAILURE when on_success raises after SUCCESS was stored - #10759

Thanks @MehrazRumman, this was a really useful review. Pushed 45a2f34 on top of the merge from main:

1. Eager mode: the new handler re-raises when eager is true, so apply() and task_always_eager behave as on main. Covered by test_trace_on_success_raises_eager_propagates.
2. after_return: I kept it in this PR since it's the same bug. It gets the same handling (log in a worker, re-raise eagerly, let Reject through). test_trace_after_return_raises checks both sides: no failure recorded for a successful task, and exactly one for a failed task, where main records two.
3. Accuracy: the description now says that key-value backends keep the stored SUCCESS and that the database backend is the one that gets overwritten. The test has a comment saying the state check doesn't pin the fix on its own, and it now asserts logger.exception.
4. Reject: an explicit except Reject: raise sits before each new handler, with a test. The lost task_failure / task_internal_error signals are now in the changelog and in the on_success / after_return docs.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.72%. Comparing base (4543e46) to head (1b4dd96).

@@           Coverage Diff           @@
##             main   #10759   +/-   ##
=======================================
  Coverage   90.72%   90.72%           
=======================================
  Files         153      153           
  Lines       21533    21549   +16     
  Branches     2651     2653    +2     
=======================================
+ Hits        19535    19551   +16     
  Misses       1685     1685           
  Partials      313      313           

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

@MehrazRumman thank you for the detailed review. The follow-up in 45a2f3420 covers the points you raised:

1. Eager mode still propagates (if eager: raise inside the new handlers).
2. after_return is wrapped the same way as on_success.
3. Description/tests: state-check comment, logger.exception assertion, and the docs/changelog notes about signals.
4. Reject is re-raised before the broad Exception handler.

I just merged current main (Changelog conflict only: Unreleased #10724 entry kept above 5.7.0rc1). Ready for another look when you have time.

@ChristianMurphy can you check this one too?

i think the unit test coverage can be improved as well @gyanu2507

I tested this with a prefork worker, against Redis and SQLite (db+sqlite) result backends, on main and on this branch. When on_success or after_return raises, the task is still SUCCESS afterward, where main overwrote it with FAILURE on SQLite. The errback doesn't run, and only task_success fires. A chord with that task in its header now runs its body, where main failed it with ChordError. A failed task whose after_return raises is stored with its own exception, not the hook's. Eager apply() matches main, and Reject, KeyboardInterrupt and SystemExit still propagate.

@gyanu2507

Before merging:

1. Changelog.rst: the merge in a3075a9 dropped the ======= under 5.7.0b1, so that heading no longer renders. I think that's what auvipy meant. There's also an extra blank line above its label.
2. trace.py: the except Exception in both new handlers now catches MemoryError, which main re-raises through the outer except MemoryError: raise. Using except (Reject, MemoryError): raise keeps that.

Also, the Reject branch for after_return has no test.

@auvipy Restored the ======= underline under the 5.7.0b1 changelog heading.

Also tightened the existing unit coverage: when on_success raises after SUCCESS is stored, the test now asserts that task_failure and task_internal_error stay quiet (in addition to handle_failure not being called and task_success still firing). Happy to add more cases if you have a specific gap in mind.

2. trace.py: the except Exception in both new handlers now catches MemoryError, which main re-raises through the outer except MemoryError: raise. Using except (Reject, MemoryError): raise keeps that.

Also, the Reject branch for after_return has no test

is this already covered? the codcov is reporting some missing coverage. not specific though, can you cross check?

I checked 34c68d3. The changelog heading renders again, and the test now checks that task_failure and task_internal_error don't fire. The docs are right about eager runs too: apply() still raises the hook's error, as on main.

These are still open from my last comment.

MemoryError is now swallowed. On main, a MemoryError raised by on_success or after_return in a worker propagates. On this branch, the new handlers log it, and the task finishes normally. Re-raising it next to Reject makes it propagate again, as on main:

except (Reject, MemoryError):
    raise

The postrun handler a few lines below already re-raises MemoryError this way.

No test raises Reject from after_return. That raise is line 673 of trace.py. It's the line Codecov reports as missing, and the tests still pass if I delete it. A version of test_trace_on_success_raises_reject_is_not_swallowed that raises from after_return would test it.

Also, the merge adds an extra blank line above .. _version-5.7.0b1:.

Addressed the remaining notes from @ChristianMurphy:

1. on_success / after_return now use except (Reject, MemoryError): raise, so MemoryError propagates again as on main.
2. Added test_trace_after_return_raises_reject_is_not_swallowed and test_trace_on_success_raises_memory_error_propagates.
3. Merged main and kept both Unreleased changelog entries (our A task whose SUCCESS is stored still fires its error callback: a pool child lost after the store, or a raising on_success #10724 note and the Redis chord Lua entry).