its-amann commented on code in PR #76908:
URL: https://github.com/apache/airflow/pull/76908#discussion_r4546657924
##########
airflow-core/src/airflow/jobs/scheduler_job_runner.py:
##########
@@ -3617,38 +3671,25 @@ def _maybe_requeue_stuck_ti(self, *, ti, session,
executor):
"The DAG or task could not be found. If a failure callback
exists, it will not be run.",
exc_info=True,
)
- else:
- if task.has_on_failure_callback:
- if inspect(ti).detached:
- ti = session.merge(ti)
- _stuck_bundle_name, _stuck_bundle_version,
_stuck_version_data = (
- _resolve_ti_callback_bundle_info(ti)
- )
- # Backfill dag_version_id for legacy tasks (Pydantic
requires uuid.UUID).
- # Note: we cannot use `continue` here because this method
is not
- # inside a loop. If backfilling fails we simply skip the
callback.
- if _ensure_ti_has_dag_version_id(ti, session, self.log):
- request = TaskCallbackRequest(
- filepath=ti.dag_model.relative_fileloc or "",
- bundle_name=_stuck_bundle_name,
- bundle_version=_stuck_bundle_version,
- version_data=_stuck_version_data,
- ti=task_instance_to_runtime(
- ti, model=TIDataModel,
map_index=coordinates.public_map_index(ti)
- ),
- msg=msg,
- context_from_server=TIRunContext(
- dag_run=ti.dag_run,
- max_tries=ti.max_tries,
- variables=[],
- connections=[],
- xcom_keys_to_clear=[],
- ),
- )
- executor.send_callback(request)
- finally:
- ti.set_state(TaskInstanceState.FAILED, session=session)
- executor.fail(executor.get_task_key(ti))
+ task = None
+ # handle_failure() reads retries and fail_fast from ti.task.
+ ti.task = task
+ needs_callback = task is not None and
(task.has_on_failure_callback or task.has_on_retry_callback)
+ # Unlike process_executor_events, a missing dag_version_id only
drops the requests here; the TI
+ # is still failed so it doesn't sit in queued.
+ _send_task_failure_requests(
+ ti,
+ task=task,
+ executor=executor,
+ msg=msg,
+ needs_callback=needs_callback,
+ coordinates=coordinates,
+ session=session,
+ log=self.log,
+ )
Review Comment:
the old `finally` here made sure the TI got failed and `executor.fail()` ran
even if building the callback request blew up. with it gone, anything raising
inside `_send_task_failure_requests` (a dag_run that won't validate into
`DRDataModel`, a missing `dag_model`) escapes the loop, since it only catches
`NotImplementedError`, and the TI sits in queued and hits the same error on
every scan. wrapping just the requests keeps that guarantee:
```suggestion
try:
_send_task_failure_requests(
ti,
task=task,
executor=executor,
msg=msg,
needs_callback=needs_callback,
coordinates=coordinates,
session=session,
log=self.log,
)
except Exception:
# Same guarantee the old finally gave: a broken callback
request must not leave the TI queued.
self.log.exception("Could not send failure requests for %s;
failing it anyway", ti)
```
fixed this plus a test in this commit, you can just cherry pick it instead
of redoing it:
https://github.com/its-amann/airflow/commit/09a7f3be5cd7569a3a163422650967bc662e0991
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
