Fix multiple_outputs no-op on deferrable KubernetesPodOperator by paultmathew · Pull Request #67226 · apache/airflow

paultmathew · 2026-05-20T06:36:17Z

Summary

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only return_value was published. Downstream tasks subscripting a key got None at runtime with no error.

Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed.

The sync path's execute_sync already returns result (pod.py:760). This aligns the deferrable path with the same contract.

Behaviour change

Success path (new): trigger_reentry returns xcom_sidecar_output. The task runner pushes return_value and, when multiple_outputs=True, also fans the dict out into per-key XComs.
Failure path (preserved): the manual xcom_push(XCOM_RETURN_KEY, ...) moves into the event["status"] != "success" branch, above the raise. Partial sidecar output is still surfaced in XCom, and the push now happens even when _clean subsequently raises (strict improvement — previously the in-finally push was unreachable in that case).

multiple_outputs fan-out is deliberately not applied on the failure path: downstream tasks that would consume per-key XComs are in UPSTREAM_FAILED anyway. The single-key return_value push on failure mirrors the sync path's failure-time push at pod.py:1198 ("Ensure that existing XCom is pushed even in case of failure").

History

The deferred-path manual push was introduced by #58488 and refined by #58998. #58488 was titled "Deferred KubernetesPodOperator pushes XCom on successful execution" — the intent was success-path only. #58998 then removed a return from the finally block (returning from finally silently swallows exceptions) and replaced it with xcom_push, preserving the symptom on both paths. The success-path fan-out was lost in that translation; this PR restores it.

Closes

Fixes#67224

Tests

Updated test_async_kpo_wait_termination_before_cleanup_on_success and test_async_kpo_wait_termination_before_cleanup_on_failure to reflect the new contract.

Added test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs to lock in the success-path return value with multiple_outputs=True.

The end-to-end _push_xcom_if_needed fan-out behavior is already covered by task-SDK unit tests.

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only `return_value` was published. Downstream tasks subscripting a key (operator.output["foo"] resolving to xcom_pull(key="foo")) got None at runtime with no error. Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed. The sync execute_sync path already returns the result for the runner to handle (pod.py:760); this aligns trigger_reentry with that same contract. The failure-path manual push is preserved by moving it inside the event["status"] != "success" branch above the raise — partial sidecar output is still surfaced in XCom when the pod fails, and the behavior is now strictly better: the push happens even when the subsequent _clean call raises (previously the in-finally push was unreachable in that case). Fixesapache#67224 Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the improvement. So it improves on one end but actually then with the fix leaves the two code path's in an un-fixed state. When either sync or sync execution fails and xcom is pushed it still skips handling multiple outputs.

Can you make it completely consistent?

Copilot

Pull request overview

Fixes deferrable KubernetesPodOperator to return the XCom sidecar payload on success so the task runner can apply _push_xcom_if_needed (including multiple_outputs fan-out), aligning behavior with the sync execution path.

Changes:

Update trigger_reentry to return xcom_sidecar_output on success (and stop manually pushing return_value in the success path).
Preserve failure-path behavior by manually pushing return_value before raising, ensuring partial sidecar output is still captured even if cleanup errors.
Adjust and extend unit tests to reflect/lock in the new return-vs-push contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py`	Return sidecar output on successful deferrable re-entry so task runner can handle XCom push and `multiple_outputs` fan-out; keep manual failure push.
`providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py`	Update existing async KPO tests for the new contract and add a test asserting `trigger_reentry` returns sidecar output when `multiple_outputs=True`.

Address review on apache#67226: the previous commit fixed the success path on deferrable KPO but left both sync and async failure paths still pushing only ``return_value``, silently dropping ``multiple_outputs`` fan-out. Add an operator-local ``_push_xcom_with_fan_out`` helper that mirrors the task runner's ``_push_xcom_if_needed`` and call it from both failure-path manual pushes (sync ``post_complete_action`` and async ``trigger_reentry``). All four code paths — sync success/failure, async success/failure — now honour ``multiple_outputs`` consistently. Promoting ``_push_xcom_if_needed`` to a public task-SDK helper would let other operators that manually push XCom on failure paths reuse the same logic and would be a cleaner long-term fix. Left as a follow-up since manual ``XCOM_RETURN_KEY`` pushes are essentially KPO-specific today (grep across all providers turns up only one other operator) and the local helper keeps this PR's blast radius matched to its scope. Tests: - New ``test_xcom_push_failed_pod_fans_out_for_multiple_outputs`` covers the sync failure path. - New ``test_async_trigger_reentry_failure_fans_out_for_multiple_outputs`` covers the async failure path. Also incorporate review nits: drop unnecessary comments, switch ``is`` to ``==`` for dict equality checks, and trim the historical context from the new test docstring (moved to the PR description). Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew · 2026-05-21T19:30:32Z

Thanks for the review @jscheffl. Pushed a fix with an operator-local _push_xcom_with_fan_out helper used by both sync (post_complete_action) and async (trigger_reentry) failure-path pushes — so all four code paths now honour multiple_outputs consistently. Added matching failure-path tests for both modes.

I considered promoting _push_xcom_if_needed to a public task-SDK helper as you mentioned. Manual XCOM_RETURN_KEY pushes from operator code are essentially KPO-specific today (grep across all providers only turns up one other place), so I kept the helper local to keep this PR focused — happy to file a follow-up issue for the wider task-SDK change if you'd prefer.

jscheffl · 2026-05-21T19:58:25Z

Looks almost good. One final (hopefully final) finding. Can you close the comments resolved?

If static checks are fixed alongside one comment from me then I think it is good to merge

@jscheffl

Address review from @jscheffl on apache#67226: the previous ``if not value:`` guard would skip the manual XCom push for any falsy-but-non-None payload (``False``, ``0``, ``""``, ``[]``, ``{}``) — all of which are legitimate sidecar outputs that downstream tasks may rely on. Switch to ``is None`` so only the "nothing extracted" case is skipped, matching the task runner's ``_push_xcom_if_needed`` semantics (which also guards on ``xcom_value is None``). Also fix D213 docstring style flagged by ruff (multi-line summary should start on the second line). Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the fix!

paultmathew · 2026-05-20T06:36:17Z

Summary

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only return_value was published. Downstream tasks subscripting a key got None at runtime with no error.

Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed.

The sync path's execute_sync already returns result (pod.py:760). This aligns the deferrable path with the same contract.

Behaviour change

Success path (new): trigger_reentry returns xcom_sidecar_output. The task runner pushes return_value and, when multiple_outputs=True, also fans the dict out into per-key XComs.
Failure path (preserved): the manual xcom_push(XCOM_RETURN_KEY, ...) moves into the event["status"] != "success" branch, above the raise. Partial sidecar output is still surfaced in XCom, and the push now happens even when _clean subsequently raises (strict improvement — previously the in-finally push was unreachable in that case).

multiple_outputs fan-out is deliberately not applied on the failure path: downstream tasks that would consume per-key XComs are in UPSTREAM_FAILED anyway. The single-key return_value push on failure mirrors the sync path's failure-time push at pod.py:1198 ("Ensure that existing XCom is pushed even in case of failure").

History

The deferred-path manual push was introduced by #58488 and refined by #58998. #58488 was titled "Deferred KubernetesPodOperator pushes XCom on successful execution" — the intent was success-path only. #58998 then removed a return from the finally block (returning from finally silently swallows exceptions) and replaced it with xcom_push, preserving the symptom on both paths. The success-path fan-out was lost in that translation; this PR restores it.

Closes

Fixes#67224

Tests

Updated test_async_kpo_wait_termination_before_cleanup_on_success and test_async_kpo_wait_termination_before_cleanup_on_failure to reflect the new contract.

Added test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs to lock in the success-path return value with multiple_outputs=True.

The end-to-end _push_xcom_if_needed fan-out behavior is already covered by task-SDK unit tests.

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only `return_value` was published. Downstream tasks subscripting a key (operator.output["foo"] resolving to xcom_pull(key="foo")) got None at runtime with no error. Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed. The sync execute_sync path already returns the result for the runner to handle (pod.py:760); this aligns trigger_reentry with that same contract. The failure-path manual push is preserved by moving it inside the event["status"] != "success" branch above the raise — partial sidecar output is still surfaced in XCom when the pod fails, and the behavior is now strictly better: the push happens even when the subsequent _clean call raises (previously the in-finally push was unreachable in that case). Fixesapache#67224 Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the improvement. So it improves on one end but actually then with the fix leaves the two code path's in an un-fixed state. When either sync or sync execution fails and xcom is pushed it still skips handling multiple outputs.

Can you make it completely consistent?

Copilot

Pull request overview

Fixes deferrable KubernetesPodOperator to return the XCom sidecar payload on success so the task runner can apply _push_xcom_if_needed (including multiple_outputs fan-out), aligning behavior with the sync execution path.

Changes:

Update trigger_reentry to return xcom_sidecar_output on success (and stop manually pushing return_value in the success path).
Preserve failure-path behavior by manually pushing return_value before raising, ensuring partial sidecar output is still captured even if cleanup errors.
Adjust and extend unit tests to reflect/lock in the new return-vs-push contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py`	Return sidecar output on successful deferrable re-entry so task runner can handle XCom push and `multiple_outputs` fan-out; keep manual failure push.
`providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py`	Update existing async KPO tests for the new contract and add a test asserting `trigger_reentry` returns sidecar output when `multiple_outputs=True`.

Address review on apache#67226: the previous commit fixed the success path on deferrable KPO but left both sync and async failure paths still pushing only ``return_value``, silently dropping ``multiple_outputs`` fan-out. Add an operator-local ``_push_xcom_with_fan_out`` helper that mirrors the task runner's ``_push_xcom_if_needed`` and call it from both failure-path manual pushes (sync ``post_complete_action`` and async ``trigger_reentry``). All four code paths — sync success/failure, async success/failure — now honour ``multiple_outputs`` consistently. Promoting ``_push_xcom_if_needed`` to a public task-SDK helper would let other operators that manually push XCom on failure paths reuse the same logic and would be a cleaner long-term fix. Left as a follow-up since manual ``XCOM_RETURN_KEY`` pushes are essentially KPO-specific today (grep across all providers turns up only one other operator) and the local helper keeps this PR's blast radius matched to its scope. Tests: - New ``test_xcom_push_failed_pod_fans_out_for_multiple_outputs`` covers the sync failure path. - New ``test_async_trigger_reentry_failure_fans_out_for_multiple_outputs`` covers the async failure path. Also incorporate review nits: drop unnecessary comments, switch ``is`` to ``==`` for dict equality checks, and trim the historical context from the new test docstring (moved to the PR description). Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew · 2026-05-21T19:30:32Z

Thanks for the review @jscheffl. Pushed a fix with an operator-local _push_xcom_with_fan_out helper used by both sync (post_complete_action) and async (trigger_reentry) failure-path pushes — so all four code paths now honour multiple_outputs consistently. Added matching failure-path tests for both modes.

I considered promoting _push_xcom_if_needed to a public task-SDK helper as you mentioned. Manual XCOM_RETURN_KEY pushes from operator code are essentially KPO-specific today (grep across all providers only turns up one other place), so I kept the helper local to keep this PR focused — happy to file a follow-up issue for the wider task-SDK change if you'd prefer.

jscheffl · 2026-05-21T19:58:25Z

Looks almost good. One final (hopefully final) finding. Can you close the comments resolved?

If static checks are fixed alongside one comment from me then I think it is good to merge

@jscheffl

Address review from @jscheffl on apache#67226: the previous ``if not value:`` guard would skip the manual XCom push for any falsy-but-non-None payload (``False``, ``0``, ``""``, ``[]``, ``{}``) — all of which are legitimate sidecar outputs that downstream tasks may rely on. Switch to ``is None`` so only the "nothing extracted" case is skipped, matching the task runner's ``_push_xcom_if_needed`` semantics (which also guards on ``xcom_value is None``). Also fix D213 docstring style flagged by ruff (multi-line summary should start on the second line). Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the fix!

paultmathew · 2026-05-20T06:36:17Z

Summary

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only return_value was published. Downstream tasks subscripting a key got None at runtime with no error.

Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed.

The sync path's execute_sync already returns result (pod.py:760). This aligns the deferrable path with the same contract.

Behaviour change

Success path (new): trigger_reentry returns xcom_sidecar_output. The task runner pushes return_value and, when multiple_outputs=True, also fans the dict out into per-key XComs.
Failure path (preserved): the manual xcom_push(XCOM_RETURN_KEY, ...) moves into the event["status"] != "success" branch, above the raise. Partial sidecar output is still surfaced in XCom, and the push now happens even when _clean subsequently raises (strict improvement — previously the in-finally push was unreachable in that case).

multiple_outputs fan-out is deliberately not applied on the failure path: downstream tasks that would consume per-key XComs are in UPSTREAM_FAILED anyway. The single-key return_value push on failure mirrors the sync path's failure-time push at pod.py:1198 ("Ensure that existing XCom is pushed even in case of failure").

History

The deferred-path manual push was introduced by #58488 and refined by #58998. #58488 was titled "Deferred KubernetesPodOperator pushes XCom on successful execution" — the intent was success-path only. #58998 then removed a return from the finally block (returning from finally silently swallows exceptions) and replaced it with xcom_push, preserving the symptom on both paths. The success-path fan-out was lost in that translation; this PR restores it.

Closes

Fixes#67224

Tests

Updated test_async_kpo_wait_termination_before_cleanup_on_success and test_async_kpo_wait_termination_before_cleanup_on_failure to reflect the new contract.

Added test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs to lock in the success-path return value with multiple_outputs=True.

The end-to-end _push_xcom_if_needed fan-out behavior is already covered by task-SDK unit tests.

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only `return_value` was published. Downstream tasks subscripting a key (operator.output["foo"] resolving to xcom_pull(key="foo")) got None at runtime with no error. Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed. The sync execute_sync path already returns the result for the runner to handle (pod.py:760); this aligns trigger_reentry with that same contract. The failure-path manual push is preserved by moving it inside the event["status"] != "success" branch above the raise — partial sidecar output is still surfaced in XCom when the pod fails, and the behavior is now strictly better: the push happens even when the subsequent _clean call raises (previously the in-finally push was unreachable in that case). Fixesapache#67224 Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the improvement. So it improves on one end but actually then with the fix leaves the two code path's in an un-fixed state. When either sync or sync execution fails and xcom is pushed it still skips handling multiple outputs.

Can you make it completely consistent?

Copilot

Pull request overview

Fixes deferrable KubernetesPodOperator to return the XCom sidecar payload on success so the task runner can apply _push_xcom_if_needed (including multiple_outputs fan-out), aligning behavior with the sync execution path.

Changes:

Update trigger_reentry to return xcom_sidecar_output on success (and stop manually pushing return_value in the success path).
Preserve failure-path behavior by manually pushing return_value before raising, ensuring partial sidecar output is still captured even if cleanup errors.
Adjust and extend unit tests to reflect/lock in the new return-vs-push contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py`	Return sidecar output on successful deferrable re-entry so task runner can handle XCom push and `multiple_outputs` fan-out; keep manual failure push.
`providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py`	Update existing async KPO tests for the new contract and add a test asserting `trigger_reentry` returns sidecar output when `multiple_outputs=True`.

Address review on apache#67226: the previous commit fixed the success path on deferrable KPO but left both sync and async failure paths still pushing only ``return_value``, silently dropping ``multiple_outputs`` fan-out. Add an operator-local ``_push_xcom_with_fan_out`` helper that mirrors the task runner's ``_push_xcom_if_needed`` and call it from both failure-path manual pushes (sync ``post_complete_action`` and async ``trigger_reentry``). All four code paths — sync success/failure, async success/failure — now honour ``multiple_outputs`` consistently. Promoting ``_push_xcom_if_needed`` to a public task-SDK helper would let other operators that manually push XCom on failure paths reuse the same logic and would be a cleaner long-term fix. Left as a follow-up since manual ``XCOM_RETURN_KEY`` pushes are essentially KPO-specific today (grep across all providers turns up only one other operator) and the local helper keeps this PR's blast radius matched to its scope. Tests: - New ``test_xcom_push_failed_pod_fans_out_for_multiple_outputs`` covers the sync failure path. - New ``test_async_trigger_reentry_failure_fans_out_for_multiple_outputs`` covers the async failure path. Also incorporate review nits: drop unnecessary comments, switch ``is`` to ``==`` for dict equality checks, and trim the historical context from the new test docstring (moved to the PR description). Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew · 2026-05-21T19:30:32Z

Thanks for the review @jscheffl. Pushed a fix with an operator-local _push_xcom_with_fan_out helper used by both sync (post_complete_action) and async (trigger_reentry) failure-path pushes — so all four code paths now honour multiple_outputs consistently. Added matching failure-path tests for both modes.

I considered promoting _push_xcom_if_needed to a public task-SDK helper as you mentioned. Manual XCOM_RETURN_KEY pushes from operator code are essentially KPO-specific today (grep across all providers only turns up one other place), so I kept the helper local to keep this PR focused — happy to file a follow-up issue for the wider task-SDK change if you'd prefer.

jscheffl · 2026-05-21T19:58:25Z

Looks almost good. One final (hopefully final) finding. Can you close the comments resolved?

If static checks are fixed alongside one comment from me then I think it is good to merge

@jscheffl

Address review from @jscheffl on apache#67226: the previous ``if not value:`` guard would skip the manual XCom push for any falsy-but-non-None payload (``False``, ``0``, ``""``, ``[]``, ``{}``) — all of which are legitimate sidecar outputs that downstream tasks may rely on. Switch to ``is None`` so only the "nothing extracted" case is skipped, matching the task runner's ``_push_xcom_if_needed`` semantics (which also guards on ``xcom_value is None``). Also fix D213 docstring style flagged by ruff (multi-line summary should start on the second line). Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the fix!

paultmathew · 2026-05-20T06:36:17Z

Summary

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only return_value was published. Downstream tasks subscripting a key got None at runtime with no error.

Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed.

The sync path's execute_sync already returns result (pod.py:760). This aligns the deferrable path with the same contract.

Behaviour change

Success path (new): trigger_reentry returns xcom_sidecar_output. The task runner pushes return_value and, when multiple_outputs=True, also fans the dict out into per-key XComs.
Failure path (preserved): the manual xcom_push(XCOM_RETURN_KEY, ...) moves into the event["status"] != "success" branch, above the raise. Partial sidecar output is still surfaced in XCom, and the push now happens even when _clean subsequently raises (strict improvement — previously the in-finally push was unreachable in that case).

multiple_outputs fan-out is deliberately not applied on the failure path: downstream tasks that would consume per-key XComs are in UPSTREAM_FAILED anyway. The single-key return_value push on failure mirrors the sync path's failure-time push at pod.py:1198 ("Ensure that existing XCom is pushed even in case of failure").

History

The deferred-path manual push was introduced by #58488 and refined by #58998. #58488 was titled "Deferred KubernetesPodOperator pushes XCom on successful execution" — the intent was success-path only. #58998 then removed a return from the finally block (returning from finally silently swallows exceptions) and replaced it with xcom_push, preserving the symptom on both paths. The success-path fan-out was lost in that translation; this PR restores it.

Closes

Fixes#67224

Tests

Updated test_async_kpo_wait_termination_before_cleanup_on_success and test_async_kpo_wait_termination_before_cleanup_on_failure to reflect the new contract.

Added test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs to lock in the success-path return value with multiple_outputs=True.

The end-to-end _push_xcom_if_needed fan-out behavior is already covered by task-SDK unit tests.

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only `return_value` was published. Downstream tasks subscripting a key (operator.output["foo"] resolving to xcom_pull(key="foo")) got None at runtime with no error. Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed. The sync execute_sync path already returns the result for the runner to handle (pod.py:760); this aligns trigger_reentry with that same contract. The failure-path manual push is preserved by moving it inside the event["status"] != "success" branch above the raise — partial sidecar output is still surfaced in XCom when the pod fails, and the behavior is now strictly better: the push happens even when the subsequent _clean call raises (previously the in-finally push was unreachable in that case). Fixesapache#67224 Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the improvement. So it improves on one end but actually then with the fix leaves the two code path's in an un-fixed state. When either sync or sync execution fails and xcom is pushed it still skips handling multiple outputs.

Can you make it completely consistent?

Copilot

Pull request overview

Fixes deferrable KubernetesPodOperator to return the XCom sidecar payload on success so the task runner can apply _push_xcom_if_needed (including multiple_outputs fan-out), aligning behavior with the sync execution path.

Changes:

Update trigger_reentry to return xcom_sidecar_output on success (and stop manually pushing return_value in the success path).
Preserve failure-path behavior by manually pushing return_value before raising, ensuring partial sidecar output is still captured even if cleanup errors.
Adjust and extend unit tests to reflect/lock in the new return-vs-push contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py`	Return sidecar output on successful deferrable re-entry so task runner can handle XCom push and `multiple_outputs` fan-out; keep manual failure push.
`providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py`	Update existing async KPO tests for the new contract and add a test asserting `trigger_reentry` returns sidecar output when `multiple_outputs=True`.

Address review on apache#67226: the previous commit fixed the success path on deferrable KPO but left both sync and async failure paths still pushing only ``return_value``, silently dropping ``multiple_outputs`` fan-out. Add an operator-local ``_push_xcom_with_fan_out`` helper that mirrors the task runner's ``_push_xcom_if_needed`` and call it from both failure-path manual pushes (sync ``post_complete_action`` and async ``trigger_reentry``). All four code paths — sync success/failure, async success/failure — now honour ``multiple_outputs`` consistently. Promoting ``_push_xcom_if_needed`` to a public task-SDK helper would let other operators that manually push XCom on failure paths reuse the same logic and would be a cleaner long-term fix. Left as a follow-up since manual ``XCOM_RETURN_KEY`` pushes are essentially KPO-specific today (grep across all providers turns up only one other operator) and the local helper keeps this PR's blast radius matched to its scope. Tests: - New ``test_xcom_push_failed_pod_fans_out_for_multiple_outputs`` covers the sync failure path. - New ``test_async_trigger_reentry_failure_fans_out_for_multiple_outputs`` covers the async failure path. Also incorporate review nits: drop unnecessary comments, switch ``is`` to ``==`` for dict equality checks, and trim the historical context from the new test docstring (moved to the PR description). Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew · 2026-05-21T19:30:32Z

Thanks for the review @jscheffl. Pushed a fix with an operator-local _push_xcom_with_fan_out helper used by both sync (post_complete_action) and async (trigger_reentry) failure-path pushes — so all four code paths now honour multiple_outputs consistently. Added matching failure-path tests for both modes.

I considered promoting _push_xcom_if_needed to a public task-SDK helper as you mentioned. Manual XCOM_RETURN_KEY pushes from operator code are essentially KPO-specific today (grep across all providers only turns up one other place), so I kept the helper local to keep this PR focused — happy to file a follow-up issue for the wider task-SDK change if you'd prefer.

jscheffl · 2026-05-21T19:58:25Z

Looks almost good. One final (hopefully final) finding. Can you close the comments resolved?

If static checks are fixed alongside one comment from me then I think it is good to merge

@jscheffl

Address review from @jscheffl on apache#67226: the previous ``if not value:`` guard would skip the manual XCom push for any falsy-but-non-None payload (``False``, ``0``, ``""``, ``[]``, ``{}``) — all of which are legitimate sidecar outputs that downstream tasks may rely on. Switch to ``is None`` so only the "nothing extracted" case is skipped, matching the task runner's ``_push_xcom_if_needed`` semantics (which also guards on ``xcom_value is None``). Also fix D213 docstring style flagged by ruff (multi-line summary should start on the second line). Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the fix!

paultmathew · 2026-05-20T06:36:17Z

Summary

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only return_value was published. Downstream tasks subscripting a key got None at runtime with no error.

Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed.

The sync path's execute_sync already returns result (pod.py:760). This aligns the deferrable path with the same contract.

Behaviour change

Success path (new): trigger_reentry returns xcom_sidecar_output. The task runner pushes return_value and, when multiple_outputs=True, also fans the dict out into per-key XComs.
Failure path (preserved): the manual xcom_push(XCOM_RETURN_KEY, ...) moves into the event["status"] != "success" branch, above the raise. Partial sidecar output is still surfaced in XCom, and the push now happens even when _clean subsequently raises (strict improvement — previously the in-finally push was unreachable in that case).

multiple_outputs fan-out is deliberately not applied on the failure path: downstream tasks that would consume per-key XComs are in UPSTREAM_FAILED anyway. The single-key return_value push on failure mirrors the sync path's failure-time push at pod.py:1198 ("Ensure that existing XCom is pushed even in case of failure").

History

The deferred-path manual push was introduced by #58488 and refined by #58998. #58488 was titled "Deferred KubernetesPodOperator pushes XCom on successful execution" — the intent was success-path only. #58998 then removed a return from the finally block (returning from finally silently swallows exceptions) and replaced it with xcom_push, preserving the symptom on both paths. The success-path fan-out was lost in that translation; this PR restores it.

Closes

Fixes#67224

Tests

Updated test_async_kpo_wait_termination_before_cleanup_on_success and test_async_kpo_wait_termination_before_cleanup_on_failure to reflect the new contract.

Added test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs to lock in the success-path return value with multiple_outputs=True.

The end-to-end _push_xcom_if_needed fan-out behavior is already covered by task-SDK unit tests.

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only `return_value` was published. Downstream tasks subscripting a key (operator.output["foo"] resolving to xcom_pull(key="foo")) got None at runtime with no error. Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed. The sync execute_sync path already returns the result for the runner to handle (pod.py:760); this aligns trigger_reentry with that same contract. The failure-path manual push is preserved by moving it inside the event["status"] != "success" branch above the raise — partial sidecar output is still surfaced in XCom when the pod fails, and the behavior is now strictly better: the push happens even when the subsequent _clean call raises (previously the in-finally push was unreachable in that case). Fixesapache#67224 Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the improvement. So it improves on one end but actually then with the fix leaves the two code path's in an un-fixed state. When either sync or sync execution fails and xcom is pushed it still skips handling multiple outputs.

Can you make it completely consistent?

Copilot

Pull request overview

Fixes deferrable KubernetesPodOperator to return the XCom sidecar payload on success so the task runner can apply _push_xcom_if_needed (including multiple_outputs fan-out), aligning behavior with the sync execution path.

Changes:

Update trigger_reentry to return xcom_sidecar_output on success (and stop manually pushing return_value in the success path).
Preserve failure-path behavior by manually pushing return_value before raising, ensuring partial sidecar output is still captured even if cleanup errors.
Adjust and extend unit tests to reflect/lock in the new return-vs-push contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py`	Return sidecar output on successful deferrable re-entry so task runner can handle XCom push and `multiple_outputs` fan-out; keep manual failure push.
`providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py`	Update existing async KPO tests for the new contract and add a test asserting `trigger_reentry` returns sidecar output when `multiple_outputs=True`.

Address review on apache#67226: the previous commit fixed the success path on deferrable KPO but left both sync and async failure paths still pushing only ``return_value``, silently dropping ``multiple_outputs`` fan-out. Add an operator-local ``_push_xcom_with_fan_out`` helper that mirrors the task runner's ``_push_xcom_if_needed`` and call it from both failure-path manual pushes (sync ``post_complete_action`` and async ``trigger_reentry``). All four code paths — sync success/failure, async success/failure — now honour ``multiple_outputs`` consistently. Promoting ``_push_xcom_if_needed`` to a public task-SDK helper would let other operators that manually push XCom on failure paths reuse the same logic and would be a cleaner long-term fix. Left as a follow-up since manual ``XCOM_RETURN_KEY`` pushes are essentially KPO-specific today (grep across all providers turns up only one other operator) and the local helper keeps this PR's blast radius matched to its scope. Tests: - New ``test_xcom_push_failed_pod_fans_out_for_multiple_outputs`` covers the sync failure path. - New ``test_async_trigger_reentry_failure_fans_out_for_multiple_outputs`` covers the async failure path. Also incorporate review nits: drop unnecessary comments, switch ``is`` to ``==`` for dict equality checks, and trim the historical context from the new test docstring (moved to the PR description). Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew · 2026-05-21T19:30:32Z

Thanks for the review @jscheffl. Pushed a fix with an operator-local _push_xcom_with_fan_out helper used by both sync (post_complete_action) and async (trigger_reentry) failure-path pushes — so all four code paths now honour multiple_outputs consistently. Added matching failure-path tests for both modes.

I considered promoting _push_xcom_if_needed to a public task-SDK helper as you mentioned. Manual XCOM_RETURN_KEY pushes from operator code are essentially KPO-specific today (grep across all providers only turns up one other place), so I kept the helper local to keep this PR focused — happy to file a follow-up issue for the wider task-SDK change if you'd prefer.

jscheffl · 2026-05-21T19:58:25Z

Looks almost good. One final (hopefully final) finding. Can you close the comments resolved?

If static checks are fixed alongside one comment from me then I think it is good to merge

@jscheffl

Address review from @jscheffl on apache#67226: the previous ``if not value:`` guard would skip the manual XCom push for any falsy-but-non-None payload (``False``, ``0``, ``""``, ``[]``, ``{}``) — all of which are legitimate sidecar outputs that downstream tasks may rely on. Switch to ``is None`` so only the "nothing extracted" case is skipped, matching the task runner's ``_push_xcom_if_needed`` semantics (which also guards on ``xcom_value is None``). Also fix D213 docstring style flagged by ruff (multi-line summary should start on the second line). Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the fix!

paultmathew · 2026-05-20T06:36:17Z

Summary

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only return_value was published. Downstream tasks subscripting a key got None at runtime with no error.

Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed.

The sync path's execute_sync already returns result (pod.py:760). This aligns the deferrable path with the same contract.

Behaviour change

Success path (new): trigger_reentry returns xcom_sidecar_output. The task runner pushes return_value and, when multiple_outputs=True, also fans the dict out into per-key XComs.
Failure path (preserved): the manual xcom_push(XCOM_RETURN_KEY, ...) moves into the event["status"] != "success" branch, above the raise. Partial sidecar output is still surfaced in XCom, and the push now happens even when _clean subsequently raises (strict improvement — previously the in-finally push was unreachable in that case).

multiple_outputs fan-out is deliberately not applied on the failure path: downstream tasks that would consume per-key XComs are in UPSTREAM_FAILED anyway. The single-key return_value push on failure mirrors the sync path's failure-time push at pod.py:1198 ("Ensure that existing XCom is pushed even in case of failure").

History

The deferred-path manual push was introduced by #58488 and refined by #58998. #58488 was titled "Deferred KubernetesPodOperator pushes XCom on successful execution" — the intent was success-path only. #58998 then removed a return from the finally block (returning from finally silently swallows exceptions) and replaced it with xcom_push, preserving the symptom on both paths. The success-path fan-out was lost in that translation; this PR restores it.

Closes

Fixes#67224

Tests

Updated test_async_kpo_wait_termination_before_cleanup_on_success and test_async_kpo_wait_termination_before_cleanup_on_failure to reflect the new contract.

Added test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs to lock in the success-path return value with multiple_outputs=True.

The end-to-end _push_xcom_if_needed fan-out behavior is already covered by task-SDK unit tests.

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only `return_value` was published. Downstream tasks subscripting a key (operator.output["foo"] resolving to xcom_pull(key="foo")) got None at runtime with no error. Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed. The sync execute_sync path already returns the result for the runner to handle (pod.py:760); this aligns trigger_reentry with that same contract. The failure-path manual push is preserved by moving it inside the event["status"] != "success" branch above the raise — partial sidecar output is still surfaced in XCom when the pod fails, and the behavior is now strictly better: the push happens even when the subsequent _clean call raises (previously the in-finally push was unreachable in that case). Fixesapache#67224 Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the improvement. So it improves on one end but actually then with the fix leaves the two code path's in an un-fixed state. When either sync or sync execution fails and xcom is pushed it still skips handling multiple outputs.

Can you make it completely consistent?

Copilot

Pull request overview

Fixes deferrable KubernetesPodOperator to return the XCom sidecar payload on success so the task runner can apply _push_xcom_if_needed (including multiple_outputs fan-out), aligning behavior with the sync execution path.

Changes:

Update trigger_reentry to return xcom_sidecar_output on success (and stop manually pushing return_value in the success path).
Preserve failure-path behavior by manually pushing return_value before raising, ensuring partial sidecar output is still captured even if cleanup errors.
Adjust and extend unit tests to reflect/lock in the new return-vs-push contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py`	Return sidecar output on successful deferrable re-entry so task runner can handle XCom push and `multiple_outputs` fan-out; keep manual failure push.
`providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py`	Update existing async KPO tests for the new contract and add a test asserting `trigger_reentry` returns sidecar output when `multiple_outputs=True`.

Address review on apache#67226: the previous commit fixed the success path on deferrable KPO but left both sync and async failure paths still pushing only ``return_value``, silently dropping ``multiple_outputs`` fan-out. Add an operator-local ``_push_xcom_with_fan_out`` helper that mirrors the task runner's ``_push_xcom_if_needed`` and call it from both failure-path manual pushes (sync ``post_complete_action`` and async ``trigger_reentry``). All four code paths — sync success/failure, async success/failure — now honour ``multiple_outputs`` consistently. Promoting ``_push_xcom_if_needed`` to a public task-SDK helper would let other operators that manually push XCom on failure paths reuse the same logic and would be a cleaner long-term fix. Left as a follow-up since manual ``XCOM_RETURN_KEY`` pushes are essentially KPO-specific today (grep across all providers turns up only one other operator) and the local helper keeps this PR's blast radius matched to its scope. Tests: - New ``test_xcom_push_failed_pod_fans_out_for_multiple_outputs`` covers the sync failure path. - New ``test_async_trigger_reentry_failure_fans_out_for_multiple_outputs`` covers the async failure path. Also incorporate review nits: drop unnecessary comments, switch ``is`` to ``==`` for dict equality checks, and trim the historical context from the new test docstring (moved to the PR description). Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew · 2026-05-21T19:30:32Z

Thanks for the review @jscheffl. Pushed a fix with an operator-local _push_xcom_with_fan_out helper used by both sync (post_complete_action) and async (trigger_reentry) failure-path pushes — so all four code paths now honour multiple_outputs consistently. Added matching failure-path tests for both modes.

I considered promoting _push_xcom_if_needed to a public task-SDK helper as you mentioned. Manual XCOM_RETURN_KEY pushes from operator code are essentially KPO-specific today (grep across all providers only turns up one other place), so I kept the helper local to keep this PR focused — happy to file a follow-up issue for the wider task-SDK change if you'd prefer.

jscheffl · 2026-05-21T19:58:25Z

Looks almost good. One final (hopefully final) finding. Can you close the comments resolved?

If static checks are fixed alongside one comment from me then I think it is good to merge

@jscheffl

Address review from @jscheffl on apache#67226: the previous ``if not value:`` guard would skip the manual XCom push for any falsy-but-non-None payload (``False``, ``0``, ``""``, ``[]``, ``{}``) — all of which are legitimate sidecar outputs that downstream tasks may rely on. Switch to ``is None`` so only the "nothing extracted" case is skipped, matching the task runner's ``_push_xcom_if_needed`` semantics (which also guards on ``xcom_value is None``). Also fix D213 docstring style flagged by ruff (multi-line summary should start on the second line). Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the fix!

paultmathew · 2026-05-20T06:36:17Z

Summary

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only return_value was published. Downstream tasks subscripting a key got None at runtime with no error.

Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed.

The sync path's execute_sync already returns result (pod.py:760). This aligns the deferrable path with the same contract.

Behaviour change

Success path (new): trigger_reentry returns xcom_sidecar_output. The task runner pushes return_value and, when multiple_outputs=True, also fans the dict out into per-key XComs.
Failure path (preserved): the manual xcom_push(XCOM_RETURN_KEY, ...) moves into the event["status"] != "success" branch, above the raise. Partial sidecar output is still surfaced in XCom, and the push now happens even when _clean subsequently raises (strict improvement — previously the in-finally push was unreachable in that case).

multiple_outputs fan-out is deliberately not applied on the failure path: downstream tasks that would consume per-key XComs are in UPSTREAM_FAILED anyway. The single-key return_value push on failure mirrors the sync path's failure-time push at pod.py:1198 ("Ensure that existing XCom is pushed even in case of failure").

History

The deferred-path manual push was introduced by #58488 and refined by #58998. #58488 was titled "Deferred KubernetesPodOperator pushes XCom on successful execution" — the intent was success-path only. #58998 then removed a return from the finally block (returning from finally silently swallows exceptions) and replaced it with xcom_push, preserving the symptom on both paths. The success-path fan-out was lost in that translation; this PR restores it.

Closes

Fixes#67224

Tests

Updated test_async_kpo_wait_termination_before_cleanup_on_success and test_async_kpo_wait_termination_before_cleanup_on_failure to reflect the new contract.

Added test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs to lock in the success-path return value with multiple_outputs=True.

The end-to-end _push_xcom_if_needed fan-out behavior is already covered by task-SDK unit tests.

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only `return_value` was published. Downstream tasks subscripting a key (operator.output["foo"] resolving to xcom_pull(key="foo")) got None at runtime with no error. Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed. The sync execute_sync path already returns the result for the runner to handle (pod.py:760); this aligns trigger_reentry with that same contract. The failure-path manual push is preserved by moving it inside the event["status"] != "success" branch above the raise — partial sidecar output is still surfaced in XCom when the pod fails, and the behavior is now strictly better: the push happens even when the subsequent _clean call raises (previously the in-finally push was unreachable in that case). Fixesapache#67224 Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the improvement. So it improves on one end but actually then with the fix leaves the two code path's in an un-fixed state. When either sync or sync execution fails and xcom is pushed it still skips handling multiple outputs.

Can you make it completely consistent?

Copilot

Pull request overview

Fixes deferrable KubernetesPodOperator to return the XCom sidecar payload on success so the task runner can apply _push_xcom_if_needed (including multiple_outputs fan-out), aligning behavior with the sync execution path.

Changes:

Update trigger_reentry to return xcom_sidecar_output on success (and stop manually pushing return_value in the success path).
Preserve failure-path behavior by manually pushing return_value before raising, ensuring partial sidecar output is still captured even if cleanup errors.
Adjust and extend unit tests to reflect/lock in the new return-vs-push contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py`	Return sidecar output on successful deferrable re-entry so task runner can handle XCom push and `multiple_outputs` fan-out; keep manual failure push.
`providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py`	Update existing async KPO tests for the new contract and add a test asserting `trigger_reentry` returns sidecar output when `multiple_outputs=True`.

Address review on apache#67226: the previous commit fixed the success path on deferrable KPO but left both sync and async failure paths still pushing only ``return_value``, silently dropping ``multiple_outputs`` fan-out. Add an operator-local ``_push_xcom_with_fan_out`` helper that mirrors the task runner's ``_push_xcom_if_needed`` and call it from both failure-path manual pushes (sync ``post_complete_action`` and async ``trigger_reentry``). All four code paths — sync success/failure, async success/failure — now honour ``multiple_outputs`` consistently. Promoting ``_push_xcom_if_needed`` to a public task-SDK helper would let other operators that manually push XCom on failure paths reuse the same logic and would be a cleaner long-term fix. Left as a follow-up since manual ``XCOM_RETURN_KEY`` pushes are essentially KPO-specific today (grep across all providers turns up only one other operator) and the local helper keeps this PR's blast radius matched to its scope. Tests: - New ``test_xcom_push_failed_pod_fans_out_for_multiple_outputs`` covers the sync failure path. - New ``test_async_trigger_reentry_failure_fans_out_for_multiple_outputs`` covers the async failure path. Also incorporate review nits: drop unnecessary comments, switch ``is`` to ``==`` for dict equality checks, and trim the historical context from the new test docstring (moved to the PR description). Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew · 2026-05-21T19:30:32Z

Thanks for the review @jscheffl. Pushed a fix with an operator-local _push_xcom_with_fan_out helper used by both sync (post_complete_action) and async (trigger_reentry) failure-path pushes — so all four code paths now honour multiple_outputs consistently. Added matching failure-path tests for both modes.

I considered promoting _push_xcom_if_needed to a public task-SDK helper as you mentioned. Manual XCOM_RETURN_KEY pushes from operator code are essentially KPO-specific today (grep across all providers only turns up one other place), so I kept the helper local to keep this PR focused — happy to file a follow-up issue for the wider task-SDK change if you'd prefer.

jscheffl · 2026-05-21T19:58:25Z

Looks almost good. One final (hopefully final) finding. Can you close the comments resolved?

If static checks are fixed alongside one comment from me then I think it is good to merge

@jscheffl

Address review from @jscheffl on apache#67226: the previous ``if not value:`` guard would skip the manual XCom push for any falsy-but-non-None payload (``False``, ``0``, ``""``, ``[]``, ``{}``) — all of which are legitimate sidecar outputs that downstream tasks may rely on. Switch to ``is None`` so only the "nothing extracted" case is skipped, matching the task runner's ``_push_xcom_if_needed`` semantics (which also guards on ``xcom_value is None``). Also fix D213 docstring style flagged by ruff (multi-line summary should start on the second line). Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the fix!

paultmathew · 2026-05-20T06:36:17Z

Summary

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only return_value was published. Downstream tasks subscripting a key got None at runtime with no error.

Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed.

The sync path's execute_sync already returns result (pod.py:760). This aligns the deferrable path with the same contract.

Behaviour change

Success path (new): trigger_reentry returns xcom_sidecar_output. The task runner pushes return_value and, when multiple_outputs=True, also fans the dict out into per-key XComs.
Failure path (preserved): the manual xcom_push(XCOM_RETURN_KEY, ...) moves into the event["status"] != "success" branch, above the raise. Partial sidecar output is still surfaced in XCom, and the push now happens even when _clean subsequently raises (strict improvement — previously the in-finally push was unreachable in that case).

multiple_outputs fan-out is deliberately not applied on the failure path: downstream tasks that would consume per-key XComs are in UPSTREAM_FAILED anyway. The single-key return_value push on failure mirrors the sync path's failure-time push at pod.py:1198 ("Ensure that existing XCom is pushed even in case of failure").

History

The deferred-path manual push was introduced by #58488 and refined by #58998. #58488 was titled "Deferred KubernetesPodOperator pushes XCom on successful execution" — the intent was success-path only. #58998 then removed a return from the finally block (returning from finally silently swallows exceptions) and replaced it with xcom_push, preserving the symptom on both paths. The success-path fan-out was lost in that translation; this PR restores it.

Closes

Fixes#67224

Tests

Updated test_async_kpo_wait_termination_before_cleanup_on_success and test_async_kpo_wait_termination_before_cleanup_on_failure to reflect the new contract.

Added test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs to lock in the success-path return value with multiple_outputs=True.

The end-to-end _push_xcom_if_needed fan-out behavior is already covered by task-SDK unit tests.

KubernetesPodOperator(do_xcom_push=True, multiple_outputs=True, deferrable=True) silently failed to fan out the sidecar's return.json dict into per-key XComs — only `return_value` was published. Downstream tasks subscripting a key (operator.output["foo"] resolving to xcom_pull(key="foo")) got None at runtime with no error. Root cause: trigger_reentry pushed return_value manually inside a finally block and never returned the value to the task runner, so the runner's _push_xcom_if_needed (the code that honors multiple_outputs and fans the dict out) was bypassed. The sync execute_sync path already returns the result for the runner to handle (pod.py:760); this aligns trigger_reentry with that same contract. The failure-path manual push is preserved by moving it inside the event["status"] != "success" branch above the raise — partial sidecar output is still surfaced in XCom when the pod fails, and the behavior is now strictly better: the push happens even when the subsequent _clean call raises (previously the in-finally push was unreachable in that case). Fixesapache#67224 Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the improvement. So it improves on one end but actually then with the fix leaves the two code path's in an un-fixed state. When either sync or sync execution fails and xcom is pushed it still skips handling multiple outputs.

Can you make it completely consistent?

Copilot

Pull request overview

Fixes deferrable KubernetesPodOperator to return the XCom sidecar payload on success so the task runner can apply _push_xcom_if_needed (including multiple_outputs fan-out), aligning behavior with the sync execution path.

Changes:

Update trigger_reentry to return xcom_sidecar_output on success (and stop manually pushing return_value in the success path).
Preserve failure-path behavior by manually pushing return_value before raising, ensuring partial sidecar output is still captured even if cleanup errors.
Adjust and extend unit tests to reflect/lock in the new return-vs-push contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py`	Return sidecar output on successful deferrable re-entry so task runner can handle XCom push and `multiple_outputs` fan-out; keep manual failure push.
`providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py`	Update existing async KPO tests for the new contract and add a test asserting `trigger_reentry` returns sidecar output when `multiple_outputs=True`.

Address review on apache#67226: the previous commit fixed the success path on deferrable KPO but left both sync and async failure paths still pushing only ``return_value``, silently dropping ``multiple_outputs`` fan-out. Add an operator-local ``_push_xcom_with_fan_out`` helper that mirrors the task runner's ``_push_xcom_if_needed`` and call it from both failure-path manual pushes (sync ``post_complete_action`` and async ``trigger_reentry``). All four code paths — sync success/failure, async success/failure — now honour ``multiple_outputs`` consistently. Promoting ``_push_xcom_if_needed`` to a public task-SDK helper would let other operators that manually push XCom on failure paths reuse the same logic and would be a cleaner long-term fix. Left as a follow-up since manual ``XCOM_RETURN_KEY`` pushes are essentially KPO-specific today (grep across all providers turns up only one other operator) and the local helper keeps this PR's blast radius matched to its scope. Tests: - New ``test_xcom_push_failed_pod_fans_out_for_multiple_outputs`` covers the sync failure path. - New ``test_async_trigger_reentry_failure_fans_out_for_multiple_outputs`` covers the async failure path. Also incorporate review nits: drop unnecessary comments, switch ``is`` to ``==`` for dict equality checks, and trim the historical context from the new test docstring (moved to the PR description). Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew · 2026-05-21T19:30:32Z

Thanks for the review @jscheffl. Pushed a fix with an operator-local _push_xcom_with_fan_out helper used by both sync (post_complete_action) and async (trigger_reentry) failure-path pushes — so all four code paths now honour multiple_outputs consistently. Added matching failure-path tests for both modes.

I considered promoting _push_xcom_if_needed to a public task-SDK helper as you mentioned. Manual XCOM_RETURN_KEY pushes from operator code are essentially KPO-specific today (grep across all providers only turns up one other place), so I kept the helper local to keep this PR focused — happy to file a follow-up issue for the wider task-SDK change if you'd prefer.

jscheffl · 2026-05-21T19:58:25Z

Looks almost good. One final (hopefully final) finding. Can you close the comments resolved?

If static checks are fixed alongside one comment from me then I think it is good to merge

@jscheffl

Address review from @jscheffl on apache#67226: the previous ``if not value:`` guard would skip the manual XCom push for any falsy-but-non-None payload (``False``, ``0``, ``""``, ``[]``, ``{}``) — all of which are legitimate sidecar outputs that downstream tasks may rely on. Switch to ``is None`` so only the "nothing extracted" case is skipped, matching the task runner's ``_push_xcom_if_needed`` semantics (which also guards on ``xcom_value is None``). Also fix D213 docstring style flagged by ruff (multi-line summary should start on the second line). Co-authored-by: Cursor <cursoragent@cursor.com>

jscheffl

Thanks for the fix!

boring-cyborgBot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels May 20, 2026

paultmathew force-pushed the fix/67224-kpo-deferrable-multiple-outputs branch 2 times, most recently from bc4bde8 to 70cfe3aCompare May 20, 2026 06:44

paultmathew force-pushed the fix/67224-kpo-deferrable-multiple-outputs branch from 70cfe3a to b3c1666Compare May 20, 2026 14:21

Trigger CI rerun after upload-artifact 403 flake
1d0c9ec
Co-authored-by: Cursor <cursoragent@cursor.com>

paultmathew marked this pull request as ready for review May 20, 2026 17:25

paultmathew requested review from hussein-awala, jedcunningham and jscheffl as code owners May 20, 2026 17:25

jscheffl reviewed May 20, 2026
View reviewed changes

jscheffl requested a review from Copilot May 20, 2026 20:41

Copilotstarted reviewing on behalf of jscheffl May 20, 2026 20:42 View session

CopilotAI reviewed May 20, 2026
View reviewed changes

Comment threadproviders/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py Outdated
Comment threadproviders/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py Outdated
Comment threadproviders/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py Outdated

jscheffl reviewed May 21, 2026
View reviewed changes

Comment threadproviders/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py Outdated

jscheffl approved these changes May 21, 2026
View reviewed changes

jscheffl merged commit ca05a56 into apache:mainMay 21, 2026
113 checks passed

This was referenced Jun 3, 2026
Status of testing Providers that were prepared on June 02, 2026 #67957
Closed
Status of testing Providers that were prepared on June 08, 2026 #68225
Closed