Repository navigation
Fix email callback bundle_version for unpinned Dag runs - #72370
Closed
ayanhussain81 wants to merge 2 commits into
Closed
ayanhussain81 wants to merge 2 commits into
ayanhussain81 wants to merge 2 commits into
Conversation
PR apache#66485 fixed scheduler-emitted TaskCallbackRequests (external kill, heartbeat timeout, stuck-in-queued) to source bundle_version from dag_run.bundle_version instead of DagVersion.bundle_version, so callbacks for Dags with disable_bundle_versioning=True stay unpinned and run against the same code the task did. The EmailRequest built a few lines below the TaskCallbackRequest in process_executor_events' external-kill path was not covered by that fix and still falls back to DagVersion.bundle_version whenever dag_version is set, regardless of whether dag_run.bundle_version is None. For an unpinned run, this pins the failure/retry email to a stale bundle version the run was never pinned to, causing the Dag Processor to check out an unnecessary versions/<sha>/ working tree for that email callback. Apply the same guard used at the other three call sites, and add a regression test mirroring test_external_kill_callback_bundle_version_follows_dag_run for the EmailRequest path.
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
1 task done
Contributor
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.
PR #66485 fixed scheduler-emitted
TaskCallbackRequests (external kill, heartbeattimeout, stuck-in-queued) to source
bundle_versionfromdag_run.bundle_versioninstead of
DagVersion.bundle_version, so that callbacks for Dags withdisable_bundle_versioning=Truestay unpinned and run against the same on-diskcode the task did, instead of pinning to a version the run was never pinned to.
That fix touched three call sites but missed a fourth: the
EmailRequestbuilt afew lines below the
TaskCallbackRequestinprocess_executor_events'sexternal-kill path. It still falls back to
DagVersion.bundle_versionwheneverdag_versionis set, regardless of whetherdag_run.bundle_versionisNone.For a Dag with
disable_bundle_versioning=Trueandemail_on_failure/email_on_retryconfigured, when a task is detected as externally killed, thefailure/retry email ends up pinned to a stale bundle version the run was never
pinned to — causing the Dag Processor to check out an unnecessary
versions/<sha>/working tree for that email callback (the exact class ofproblem #66485 set out to fix, just through the one path it didn't touch).
This applies the same guard used at the other three call sites (and used by the
_resolve_ti_callback_bundle_infohelper, whose own docstring saysprocess_executor_events"inlines the same resolution" — this brings it back insync), and adds a regression test mirroring the existing
test_external_kill_callback_bundle_version_follows_dag_runfor theEmailRequestpath, since the existing
TestSchedulerCallbackBundleInfoDagVersionNullablesuiteverifies a reimplementation of the logic rather than the real per-callsite code,
which is how this one diverged unnoticed.
Verified locally: the new test fails against the pre-fix code
(
AssertionError: -'abc123-sha' +None) and passes with the fix; the fullbundle_version/process_executor_events/ heartbeat-timeout test surface intest_scheduler_job.pypasses with no regressions;ruff checkandruff format --checkare clean on both changed files.Was generative AI tooling used to co-author this PR?
Claude Code was used to investigate the root cause (via
git blame/git logon theoriginal fix, and tracing
EmailRequest.bundle_versionthrough toBundleVersionLock), implement the fix, write the regression test, and run/verifythe test suite. All findings and the diff were reviewed and understood before
submission.
{pr_number}.bugfix.rst) will be added as a follow-up commit once this PR's number is known.