You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "sahilSENPAII (via GitHub)" <gi...@apache.org> on 2026/10/16 19:27:32 UTC
[PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
sahilSENPAII opened a new pull request, #74890:
URL: https://github.com/apache/airflow/pull/74890
related: #69873, #70247
### Problem
When one bulk Dag run update request (`PATCH /dags/{dag_id}/dagRuns`, action `update`) sets both `state` and `note` on a run, the `on_dag_run_success` / `on_dag_run_failed` listeners see the run's previous note instead of the note sent in the request.
### Cause
`BulkDagRunService.handle_bulk_update` calls `patch_dag_run_state()`, which fires the listeners, before `patch_dag_run_note()`. The bulk update path was added in #67948. #69873 fixed this order for the single-run PATCH route and #70247 fixed it for task instances, but neither covered the bulk Dag run path.
### Fix
Apply the note before the state in `handle_bulk_update`, matching the single-run route.
### Testing
- Added `TestBulkDagRuns.test_bulk_update_listener_sees_note_when_note_and_state_both_updated`, the bulk equivalent of the single-run test from #69873. It fails without the fix (the listener sees `None`) and passes with it.
- `airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py`: 402 passed.
- `prek run --stage pre-commit` on the changed files passed. `generate-openapi-spec` and `check-tests-in-the-right-folders` were skipped locally because they need Breeze; there is no API schema change.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Code following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "sahilSENPAII (via GitHub) via commits" <co...@airflow.apache.org>.
sahilSENPAII commented on PR #74890:
URL: https://github.com/apache/airflow/pull/74890#issuecomment-6651025603
@shahar1 thanks for approving the workflows earlier. CI is green and I've addressed the earlier review feedback (added the extra test scenarios). Would you be able to review this when you get a chance, or let me know who's best to ping for the API area?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "SameerMesiah97 (via GitHub)" <gi...@apache.org>.
SameerMesiah97 commented on code in PR #74890:
URL: https://github.com/apache/airflow/pull/74890#discussion_r4299679958
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py:
##########
@@ -5099,6 +5099,29 @@ def test_bulk_update_note_only(self, test_client, session):
assert dag_run.note == "bulk note"
assert dag_run.state == DAG1_RUN1_STATE
+ @pytest.mark.usefixtures("configure_git_connection_for_dag_bundle")
+ def test_bulk_update_listener_sees_note_when_note_and_state_both_updated(
+ self, test_client, listener_manager
+ ):
+ listener = ClassBasedListener()
+ listener_manager(listener)
+ response = test_client.patch(
+ self.ENDPOINT_URL,
+ json={
+ "actions": [
+ {
+ "action": "update",
+ "entities": [
+ {"dag_run_id": DAG1_RUN2_ID, "state": "success", "note": "listener_note"}
+ ],
+ }
+ ]
+ },
+ )
+ assert response.status_code == 200
+ assert response.json()["update"]["success"] == [f"{DAG1_ID}.{DAG1_RUN2_ID}"]
+ assert listener.dag_run_note_at_listener == "listener_note"
Review Comment:
I think you need to cover the following scenarios as well:
1) The new note overriding the old note
2) New note being `None`
3) Falsy new note
Also cover both failing and successful DAG runs. Please see the below:
```
@pytest.mark.usefixtures("configure_git_connection_for_dag_bundle")
@pytest.mark.parametrize("state", [DagRunState.SUCCESS, DagRunState.FAILED])
@pytest.mark.parametrize(
("existing_note", "note_update", "expected_note"),
[
pytest.param(
None, {"note": "new note"}, "new note", id="new-note"
),
pytest.param(
"old note", {"note": "new note"}, "new note", id="replace-stale-note"
),
pytest.param(
None, {"note": None}, None, id="none-without-existing-note"
),
pytest.param(
"old note", {"note": None}, "old note", id="none-preserves-note"
),
pytest.param(
"old note", {}, "old note", id="omitted-preserves-note"
),
pytest.param(
"old note", {"note": ""}, "", id="empty-string-replaces-note"
),
],
)
def test_bulk_update_listener_sees_expected_note(
self,
test_client,
listener_manager,
session,
state,
existing_note,
note_update,
expected_note,
):
dag_run = session.scalar(
select(DagRun).where(
DagRun.dag_id == DAG1_ID,
DagRun.run_id == DAG1_RUN2_ID,
)
)
assert dag_run is not None
dag_run.state = DagRunState.RUNNING
if existing_note is not None:
dag_run.note = (existing_note, "not_test")
session.commit()
listener = ClassBasedListener()
listener_manager(listener)
response = test_client.patch(
self.ENDPOINT_URL,
json={
"actions": [
{
"action": "update",
"entities": [
{
"dag_run_id": DAG1_RUN2_ID,
"state": state,
**note_update,
}
],
}
]
},
)
assert response.status_code == 200
assert response.json()["update"]["success"] == [
f"{DAG1_ID}.{DAG1_RUN2_ID}"
]
assert response.json()["update"]["errors"] == []
assert listener.state == [state]
assert listener.dag_run_note_at_listener == expected_note
session.expire_all()
dag_run = session.scalar(
select(DagRun).where(
DagRun.dag_id == DAG1_ID,
DagRun.run_id == DAG1_RUN2_ID,
)
)
assert dag_run.state == state
assert dag_run.note == expected_note
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "github-actions[bot] (via GitHub) via commits" <co...@airflow.apache.org>.
github-actions[bot] commented on PR #74890:
URL: https://github.com/apache/airflow/pull/74890#issuecomment-6651255679
Hi maintainer, this PR was merged without a milestone set.
We've automatically set the milestone to **[Airflow 3.4.1](https://github.com/apache/airflow/milestone/154)** based on: backport label targeting v3-4-test
If this milestone is not correct, please update it to the appropriate milestone.
> This comment was generated by [Milestone Tag Assistant](https://github.com/apache/airflow/blob/main/.github/workflows/milestone-tag-assistant.yml).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "shahar1 (via GitHub) via commits" <co...@airflow.apache.org>.
shahar1 merged PR #74890:
URL: https://github.com/apache/airflow/pull/74890
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "sahilSENPAII (via GitHub)" <gi...@apache.org>.
sahilSENPAII commented on code in PR #74890:
URL: https://github.com/apache/airflow/pull/74890#discussion_r4300180523
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py:
##########
@@ -5099,6 +5099,29 @@ def test_bulk_update_note_only(self, test_client, session):
assert dag_run.note == "bulk note"
assert dag_run.state == DAG1_RUN1_STATE
+ @pytest.mark.usefixtures("configure_git_connection_for_dag_bundle")
+ def test_bulk_update_listener_sees_note_when_note_and_state_both_updated(
+ self, test_client, listener_manager
+ ):
+ listener = ClassBasedListener()
+ listener_manager(listener)
+ response = test_client.patch(
+ self.ENDPOINT_URL,
+ json={
+ "actions": [
+ {
+ "action": "update",
+ "entities": [
+ {"dag_run_id": DAG1_RUN2_ID, "state": "success", "note": "listener_note"}
+ ],
+ }
+ ]
+ },
+ )
+ assert response.status_code == 200
+ assert response.json()["update"]["success"] == [f"{DAG1_ID}.{DAG1_RUN2_ID}"]
+ assert listener.dag_run_note_at_listener == "listener_note"
Review Comment:
Thanks for the review.
Updated the test to cover success/failed, a new note, replacing an old note, and an empty note. An empty note clears the note, so the expected value there is None instead of "".
Skipped the None/omitted-note cases since they don't hit patch_dag_run_note, so they pass with or without this fix. Can add them if needed.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "github-actions[bot] (via GitHub) via commits" <co...@airflow.apache.org>.
github-actions[bot] commented on PR #74890:
URL: https://github.com/apache/airflow/pull/74890#issuecomment-6651259511
### Backport successfully created: v3-4-test
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>✅</td>
<td>v3-4-test</td>
<td><a href="https://github.com/apache/airflow/pull/76523"><img src="https://img.shields.io/badge/PR-76523-blue" alt="PR Link"></a></td>
</tr>
</table>
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "fat-catTW (via GitHub)" <gi...@apache.org>.
fat-catTW commented on code in PR #74890:
URL: https://github.com/apache/airflow/pull/74890#discussion_r4303231334
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/dag_run.py:
##########
@@ -486,11 +486,12 @@ def handle_bulk_update(
continue
dag_id, run_id = key
dag_run = dag_run_map[key]
+ # Apply "note" before "state" so listeners fired inside patch_dag_run_state() see the updated note.
+ if entity.note is not None:
Review Comment:
Thanks for the PR.
Maybe we should resolve the Dag before updating its note when `state` is set?
If `get_dag_for_run()` raises 404, the bulk handler catches it and the request still commits the new note even though the state update failed.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "sahilSENPAII (via GitHub)" <gi...@apache.org>.
sahilSENPAII commented on code in PR #74890:
URL: https://github.com/apache/airflow/pull/74890#discussion_r4303584091
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/dag_run.py:
##########
@@ -486,11 +486,12 @@ def handle_bulk_update(
continue
dag_id, run_id = key
dag_run = dag_run_map[key]
+ # Apply "note" before "state" so listeners fired inside patch_dag_run_state() see the updated note.
+ if entity.note is not None:
Review Comment:
yep worked on it and pushed.
Moved the Dag lookup to the top like the single-run route does, so the note doesn't get saved if the lookup fails. Added a test for it too.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply note before state in bulk Dag run update so listeners see it [airflow]
Posted by "boring-cyborg[bot] (via GitHub) via commits" <co...@airflow.apache.org>.
boring-cyborg[bot] commented on PR #74890:
URL: https://github.com/apache/airflow/pull/74890#issuecomment-6651231892
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org