Skip to content

Select deadlines by id so that deadlines associated with other dagruns of same run_id are not pruned. #58574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
potiuk merged 2 commits into from
Dec 1, 2025
Merged

Select deadlines by id so that deadlines associated with other dagruns of same run_id are not pruned. #58574

potiuk merged 2 commits into from
Dec 1, 2025

Conversation

@tirkarthi
Copy link
Contributor

@tirkarthi tirkarthi commented Nov 22, 2025 •
edited
Loading

Filtering by DagRun.run_id results in fetching other dagruns of same run_id which could be in running state with end_date as None. Filter by DagRun.id to get only the dagrun relevant and prune deadlines associated with it.

Closes #58560

@tirkarthi tirkarthi requested a review from ferruzzi November 22, 2025 06:37
@tirkarthi
Copy link
Contributor Author

tirkarthi commented Nov 22, 2025

For some reason session.merge is required for tests but the patch works without it in the scheduler with end_date set. Please add in if I am missing something in test setup.

@tirkarthi tirkarthi mentioned this pull request Nov 25, 2025
Closed
2 tasks
@ferruzzi
Copy link
Contributor

ferruzzi commented Nov 25, 2025

You've said you were able to recreate the issue; I presume you've tested this change and it fixes the issue?

@tirkarthi
Copy link
Contributor Author

tirkarthi commented Nov 25, 2025

Yes, the tests also fail when the query is changed giving the traceback as reported in the issue. The issue can be reproduced with 2 dags with a deadline and task of time.sleep(10) and time.sleep(100) respectively
When dag1 completes it tries to prune deadline for dag2 of same run_id. This is fixed by using DagRun.id to get deadline associated with only the dagrun and avoids retrieving other dagruns of same run_id.

My question was that tests required session.merge but testing it manually in the UI with above setup didn't require it.

@ferruzzi
Copy link
Contributor

ferruzzi commented Nov 25, 2025

I'm a little scattered right now, just got back from vacation and getting pulled in a few directions. I THINK the issue is that the dag_maker fixture doesn't merge. Try the same tests but creating the DAG from scratch like some of the others in that file do. I'm pretty sure that was the fix when I ran into it in the past.

@ramitkataria
Copy link
Contributor

ramitkataria commented Nov 27, 2025

@amoghrajesh @potiuk can we please have this backported for 3.1.4 once it is merged?

ramitkataria
ramitkataria approved these changes Nov 27, 2025
View reviewed changes
Copy link
Contributor

@ramitkataria ramitkataria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the current thread about the session.merge, looks good to me!

@tirkarthi tirkarthi added the backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch label Nov 29, 2025
@tirkarthi
Copy link
Contributor Author

tirkarthi commented Nov 29, 2025

I took some from help from AI and looked at test_dagrun_set_state_end_date which was testing end_date with same flow. It seems I need to add session.add(dag_run) so that the updates to the attribute are tracked. I removed the session.merge in core and adding session.add(dag_run) in tests seems to fix it. print(vars(inspect(dag_run1)), vars(session.identity_map)) before and after session.add was helpful.

https://docs.sqlalchemy.org/en/20/orm/session_state_management.html#session-object-states

@tirkarthi tirkarthi added this to the Airflow 3.1.4 milestone Nov 29, 2025
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Dec 1, 2025
ferruzzi
ferruzzi approved these changes Dec 1, 2025
View reviewed changes
Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@potiuk
Copy link
Member

potiuk commented Dec 1, 2025

Make sense!

@potiuk potiuk merged commit c9d0fab into apache:main Dec 1, 2025
65 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 1, 2025
@tirkarthi
[v3-1-test] Select deadlines by id so that deadlines associated with …
9e16769 
…other dagruns of same run_id are not pruned. (#58574)

* Select deadlines by id so that deadlines associated with other dagruns of same run_id are not pruned.

* Track updates to dagrun attribute by session.add .
(cherry picked from commit c9d0fab)

Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
@github-actions
Copy link

github-actions bot commented Dec 1, 2025

Backport successfully created: v3-1-test

Status Branch Result
✅ v3-1-test PR Link

potiuk pushed a commit that referenced this pull request Dec 1, 2025
@github-actions @tirkarthi
[v3-1-test] Select deadlines by id so that deadlines associated with …
64642c2 
…other dagruns of same run_id are not pruned. (#58574) (#58910)

* Select deadlines by id so that deadlines associated with other dagruns of same run_id are not pruned.

* Track updates to dagrun attribute by session.add .
(cherry picked from commit c9d0fab)

Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
ephraimbuddy pushed a commit that referenced this pull request Dec 3, 2025
@github-actions @tirkarthi @ephraimbuddy
[v3-1-test] Select deadlines by id so that deadlines associated with …
1ba88b7 
…other dagruns of same run_id are not pruned. (#58574) (#58910)

* Select deadlines by id so that deadlines associated with other dagruns of same run_id are not pruned.

* Track updates to dagrun attribute by session.add .
(cherry picked from commit c9d0fab)

Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
RoyLee1224 pushed a commit to RoyLee1224/airflow that referenced this pull request Dec 3, 2025
@tirkarthi @RoyLee1224
Select deadlines by id so that deadlines associated with other dagrun…
18d060f 
…s of same run_id are not pruned. (apache#58574)

* Select deadlines by id so that deadlines associated with other dagruns of same run_id are not pruned.

* Track updates to dagrun attribute by session.add .
Copilot AI pushed a commit to jason810496/airflow that referenced this pull request Dec 5, 2025
@tirkarthi
Select deadlines by id so that deadlines associated with other dagrun…
defc69f 
…s of same run_id are not pruned. (apache#58574)

* Select deadlines by id so that deadlines associated with other dagruns of same run_id are not pruned.

* Track updates to dagrun attribute by session.add .
itayweb pushed a commit to itayweb/airflow that referenced this pull request Dec 6, 2025
@tirkarthi @itayweb
Select deadlines by id so that deadlines associated with other dagrun…
8285087 
…s of same run_id are not pruned. (apache#58574)

* Select deadlines by id so that deadlines associated with other dagruns of same run_id are not pruned.

* Track updates to dagrun attribute by session.add .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@ferruzzi ferruzzi ferruzzi approved these changes

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

+1 more reviewer

@ramitkataria ramitkataria ramitkataria approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch type:bug-fix Changelog: Bug Fixes

Projects

None yet

Milestone

Airflow 3.1.4

Development

Successfully merging this pull request may close these issues.

Scheduler pod crash because of Deadline set on DAG

5 participants

@tirkarthi @ferruzzi @ramitkataria @potiuk @ephraimbuddy