You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "GayathriSrividya (via GitHub)" <gi...@apache.org> on 2026/06/04 06:34:18 UTC
[PR] Execution API: Resolve retry callback team via TI history [airflow]
GayathriSrividya opened a new pull request, #67998:
URL: https://github.com/apache/airflow/pull/67998
closes #67923
When multi-team mode is enabled, execution API team resolution now falls back to TaskInstanceHistory when the current TaskInstance row is no longer available for retry callbacks.
The fix updates the team-name statement to union TaskInstance and TaskInstanceHistory lookup paths and return the first match.
It also adds regression tests that cover:
- multi-team on/off behavior in get_team_name_for_ti
- statement shape including both task_instance and task_instance_history lookup paths
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — GitHub Copilot (GPT-5.3-Codex)
Generated-by: GitHub Copilot (GPT-5.3-Codex) following https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-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
Re: [PR] Execution API: Resolve retry callback team via TI history [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #67998: Execution API: Resolve retry callback team via TI history
URL: https://github.com/apache/airflow/pull/67998
--
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] Execution API: Resolve retry callback team via TI history [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #67998:
URL: https://github.com/apache/airflow/pull/67998#issuecomment-4798345260
@GayathriSrividya — I've removed the `ready for maintainer review` label because the next step here is yours: this branch now has **merge conflicts with `main`**. It'll go back into the maintainer queue automatically once you rebase onto the latest `main` and push — no need to re-add the label by hand. There's no rush.
<sub>Automated triage note drafted by an AI-assisted tool — may get things wrong; a real Apache Airflow maintainer takes the next look once it's green. ([why automated](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md))</sub>
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
--
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] Execution API: Resolve retry callback team via TI history [airflow]
Posted by "GayathriSrividya (via GitHub)" <gi...@apache.org>.
GayathriSrividya commented on PR #67998:
URL: https://github.com/apache/airflow/pull/67998#issuecomment-4967530768
> @GayathriSrividya — I've removed the `ready for maintainer review` label because the next step here is yours: this branch now has **merge conflicts with `main`**. It'll go back into the maintainer queue automatically once you rebase onto the latest `main` and push — no need to re-add the label by hand. There's no rush.
>
> Automated triage note drafted by an AI-assisted tool — may get things wrong; a real Apache Airflow maintainer takes the next look once it's green. ([why automated](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md))
>
> Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
I’ve rebased this onto the latest main commit to branch. It should be back in sync now.
--
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] Execution API: Resolve retry callback team via TI history [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #67998:
URL: https://github.com/apache/airflow/pull/67998#discussion_r3680764936
##########
airflow-core/tests/unit/api_fastapi/execution_api/test_security.py:
##########
@@ -273,3 +275,36 @@ async def test_returns_none_without_session_when_multi_team_disabled(self):
assert result is None
mock_create_session.assert_not_called()
+
+
+class TestGetTeamNameForTI:
+ @pytest.mark.parametrize(
+ ("multi_team", "expected_result"),
+ [
+ (True, "team_a"),
+ (False, None),
+ ],
+ )
+ def test_get_team_name_for_ti_respects_multi_team(self, multi_team, expected_result):
+ session = MagicMock()
+ session.scalar.return_value = "team_a"
+
+ with patch("airflow.configuration.conf.getboolean", return_value=multi_team):
+ result = get_team_name_for_ti(UUID("d9edb890-fa95-4049-b66f-b6469d4fbc32"), session)
+
+ assert result == expected_result
+ if multi_team:
+ session.scalar.assert_called_once()
+ else:
+ session.scalar.assert_not_called()
+
+
+class TestTeamNameStatement:
+ def test_team_name_statement_checks_task_instance_and_history(self):
Review Comment:
Asserting `"UNION ALL" in rendered` and `"FROM task_instance_history" in rendered` tests the SQL text rather than the behaviour — it would pass on a statement that compiles correctly but resolves the wrong team, and it'll break on any cosmetic change to how SQLAlchemy renders the query.
What would actually protect this path is a DB-backed test: create a Dag on a bundle owned by a team, archive a try into `task_instance_history`, then assert `get_team_name_for_ti(...)` returns that team — and returns `None` for a TI whose bundle has no team. Given this decides authorization scope, behavioural coverage seems worth the extra setup.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/tests/unit/api_fastapi/execution_api/test_security.py:
##########
@@ -273,3 +275,36 @@ async def test_returns_none_without_session_when_multi_team_disabled(self):
assert result is None
mock_create_session.assert_not_called()
+
+
+class TestGetTeamNameForTI:
+ @pytest.mark.parametrize(
+ ("multi_team", "expected_result"),
+ [
+ (True, "team_a"),
+ (False, None),
+ ],
+ )
+ def test_get_team_name_for_ti_respects_multi_team(self, multi_team, expected_result):
+ session = MagicMock()
+ session.scalar.return_value = "team_a"
+
+ with patch("airflow.configuration.conf.getboolean", return_value=multi_team):
Review Comment:
`patch("airflow.configuration.conf.getboolean", return_value=multi_team)` replaces *every* `getboolean` call for the duration, not just `core/multi_team` — so anything else consulting a boolean config inside this block silently gets the parametrized value too.
Airflow's convention here is `conf_vars` from `tests_common.test_utils.config`, which scopes the override to the specific key. Since the value varies with `@pytest.mark.parametrize`, use it as a context manager:
```python
with conf_vars({("core", "multi_team"): str(multi_team)}):
result = get_team_name_for_ti(UUID(...), session)
```
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/execution_api/security.py:
##########
@@ -275,6 +276,17 @@ def _team_name_for_ti_stmt(ti_id):
.where(TaskInstance.id == ti_id)
)
+ task_instance_history_stmt = (
+ select(Team.name)
+ .select_from(TaskInstanceHistory)
+ .join(DagModel, DagModel.dag_id == TaskInstanceHistory.dag_id)
+ .join(DagBundleModel, DagBundleModel.name == DagModel.bundle_name)
+ .join(DagBundleModel.teams)
+ .where(TaskInstanceHistory.task_instance_id == ti_id)
+ )
+
+ return union_all(task_instance_stmt, task_instance_history_stmt)
Review Comment:
Worth a thought on `union_all` vs `union`. `task_instance_id` is the sole primary key of `task_instance_history`, so this is at most two rows — one live, one archived — rather than one per try, which is much better than I first assumed.
But for a retried TI both branches do match, and the caller uses `session.scalar()`, which takes the first row with no `ORDER BY`. Normally both resolve to the same team, so it's benign; if a Dag ever moved between bundles owned by different teams, the answer becomes whichever row the backend happened to return first. `union` would collapse identical values and make the duplicate case explicit if it ever isn't.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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] Execution API: Resolve retry callback team via TI history [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #67998:
URL: https://github.com/apache/airflow/pull/67998#issuecomment-5839363867
Hello @GayathriSrividya - thank you for your contributions to Apache Airflow!
The Airflow community has introduced a limit of **5 open pull requests at a time** for contributors without write access to the repository. You currently have 7 open pull requests, so - as a one-time step of introducing the limit - we closed the ones where maintainers have not engaged yet:
* #67831 - Fix missing task instance history for non-running retries
* #67862 - Fix AttributeError when notifier renders templates with SerializedDAG
* #67998 - Execution API: Resolve retry callback team via TI history
* #68125 - Fix backfill completion race during creation
* #69878 - Show clear error for invalid sql_alchemy_conn at startup
* #69972 - Fix dag parsing persistence when bundle cache is missing after processor restart
These pull requests stay open because maintainers are already engaged in them - they count towards your limit:
* #68499 - Fix JWT token not refreshed when token expires mid-request
This is **not** a judgement of you or of your changes. We never told contributors before that opening many pull requests at once was a problem, so there is nothing to feel bad about - and nothing is lost: your branches, commits and the review history stay where they are.
What we ask you to do is to make your **first prioritization decision**: choose which of the pull requests above matter most to you, and reopen them (up to 5 open at a time, including the ones still open) with the "Reopen pull request" button or `gh pr reopen <PR_NUMBER> --repo apache/airflow`. Reopen the ones you are ready to follow through - keep them rebased, respond to review comments and fix failing checks.
While your pull requests are waiting for review, the most valuable thing you can do is help in other ways - reviewing other contributors' pull requests, helping with issues, and taking part in the discussions on the devlist and Slack.
Why we introduced the limit, what it means for you and how to reopen or restore a pull request is explained in https://github.com/apache/airflow/blob/main/contributing-docs/32_open_pull_request_limit.rst.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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