You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ferruzzi (via GitHub)" <gi...@apache.org> on 2026/08/21 22:54:13 UTC
[PR] Allow Variable.get to reuse the caller's database session [airflow]
ferruzzi opened a new pull request, #71968:
URL: https://github.com/apache/airflow/pull/71968
`MetastoreBackend.get_variable` is decorated with `@provide_session`. Called without a session it goes through `create_session()`, which for a scoped session returns *the caller's own session* and commits it on exit. So any code that reads a Variable while holding a transaction gets that transaction committed underneath it — detaching its objects, or raising `UNEXPECTED COMMIT` under the scheduler's `prohibit_commit` guard, where the error is then swallowed per-backend and surfaces as a missing Variable.
`Variable.get` and `Variable.get_variable_from_secrets` now take an optional keyword-only `session`, forwarded only to `MetastoreBackend`. `Variable.update` forwards its own.
Affected today, all reached from `_create_dagruns_for_dags` inside the guard:
* Deadline Alerts using `VariableInterval`
* Custom timetables reading a Variable in `next_dagrun_info` — `next_dagrun` is left NULL, so the Dag is never eligible and never runs, with nothing logged
* Dag sync via `update_dags`; `Variable.update`; `Variable.setdefault`
Scope: this fixes the core read path only. `airflow.sdk.Variable.get` and `Connection.get_connection_from_secrets` share the defect and are unchanged, so callers going through those are still affected. #68917 can drop its duplicated backend walk once this lands.
closes: #71801
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [X] Yes (please specify the tool below)
<!--
Generated-by: Kiro (Claude Opus 5) 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] Allow Variable.get to reuse the caller's database session [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #71968:
URL: https://github.com/apache/airflow/pull/71968
--
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] Allow Variable.get to reuse the caller's database session [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #71968:
URL: https://github.com/apache/airflow/pull/71968#discussion_r3844734215
##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -487,7 +509,16 @@ def get_variable_from_secrets(key: str, team_name: str | None = None) -> str | N
for secrets_backend in ensure_secrets_loaded():
try:
var_val = call_secrets_backend_method(
- secrets_backend.get_variable, team_name=team_name, key=key
+ secrets_backend.get_variable,
+ team_name=team_name,
+ key=key,
+ # Only the metastore backend touches the metadata database, and it is the only
+ # one whose signature accepts a session.
+ **(
+ {"session": session}
+ if session is not None and isinstance(secrets_backend, MetastoreBackend)
+ else {}
+ ),
Review Comment:
For readability purposes I would probably create a variable for that
--
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] Allow Variable.get to reuse the caller's database session [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #71968:
URL: https://github.com/apache/airflow/pull/71968#discussion_r3845377578
##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -487,7 +509,16 @@ def get_variable_from_secrets(key: str, team_name: str | None = None) -> str | N
for secrets_backend in ensure_secrets_loaded():
try:
var_val = call_secrets_backend_method(
- secrets_backend.get_variable, team_name=team_name, key=key
+ secrets_backend.get_variable,
+ team_name=team_name,
+ key=key,
+ # Only the metastore backend touches the metadata database, and it is the only
+ # one whose signature accepts a session.
+ **(
+ {"session": session}
+ if session is not None and isinstance(secrets_backend, MetastoreBackend)
+ else {}
+ ),
Review Comment:
That's fair, I do tend to overuse comprehensions. I'll make the change.
--
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] Allow Variable.get to reuse the caller's database session [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #71968:
URL: https://github.com/apache/airflow/pull/71968#discussion_r3867423327
##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -138,13 +146,20 @@ def setdefault(cls, key, default, description=None, deserialize_json=False):
:param description: Default value to set Description of the Variable
:param deserialize_json: Store this as a JSON encoded value in the DB
and un-encode it when retrieving a value
- :param session: Session
+ :param session: Existing session to reuse for the metadata database read and write.
+ Callers holding an open transaction must pass it.
Review Comment:
On hold until #71802 merges. That PR adds `SerializedVariableInterval.resolve()`, which reads a Variable while the scheduler is already holding a session, so it becomes the first real caller for
this parameter. Once it lands I'll add `session` to `resolve()` and pass it through to `Variable.get()` here, rather than merging a parameter with no consumers.
--
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] Allow Variable.get to reuse the caller's database session [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #71968:
URL: https://github.com/apache/airflow/pull/71968#discussion_r3848158886
##########
airflow-core/newsfragments/71968.bugfix.rst:
##########
@@ -0,0 +1 @@
+``Variable.get``, ``Variable.get_variable_from_secrets`` and ``Variable.setdefault`` now accept an optional keyword-only ``session`` that is forwarded to the metastore secrets backend, so a lookup made while holding an open session (most notably inside the scheduler's ``prohibit_commit`` guard) reuses the caller's transaction instead of opening a scoped session and committing it; ``Variable.update`` now forwards its session to its own existence check, which had the same problem.
Review Comment:
```suggestion
``Variable.get``, ``Variable.get_variable_from_secrets`` and ``Variable.setdefault`` now accept an optional keyword-only ``session`` that is forwarded to the metastore secrets backend. A lookup made while holding an open session (most notably inside the scheduler's ``prohibit_commit`` guard) now reuses the caller's transaction instead of opening a scoped session and committing it. ``Variable.update`` now forwards its session to its own existence check, which had the same problem.
```
##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -485,9 +515,17 @@ def get_variable_from_secrets(key: str, team_name: str | None = None) -> str | N
var_val = None
# iterate over backends if not in cache (or expired)
for secrets_backend in ensure_secrets_loaded():
+ # Only the metastore backend touches the metadata database, and it is the only
Review Comment:
Nit: This comment line confused me at first. I think this clarifies it a bit.
```suggestion
# Only the metastore variable backend touches the metadata database, and it is the only
```
--
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