Allow Variable.get to reuse the caller's database session - #71968
Draft
ferruzzi wants to merge 8 commits into
Draft
Allow Variable.get to reuse the caller's database session#71968ferruzzi wants to merge 8 commits into
ferruzzi wants to merge 8 commits into
Conversation
`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. apache#68917 can drop its duplicated backend walk once this lands. closes: apache#71801
ferruzzi
marked this pull request as ready for review
August 21, 2026 23:27
vincbeck
reviewed
Aug 24, 2026
o-nikolas
reviewed
Aug 24, 2026
ferruzzi
commented
Aug 26, 2026
ferruzzi
left a comment
Contributor
Author
There was a problem hiding this comment.
Left a blocking comment to prevent accidental merge. PR #71802 will present a merge conflict and it will be easier to resolve if that one merges before this one. Also moving this to draft just in case, but please leave that comment unresolved as an enforced "do not merge" blocker.
Comment on lines
+149
to
+150
| :param session: Existing session to reuse for the metadata database read and write. | ||
| Callers holding an open transaction must pass it. |
Contributor
Author
There was a problem hiding this 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.
ferruzzi
marked this pull request as draft
August 26, 2026 23:13
This was referenced Aug 27, 2026
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.
MetastoreBackend.get_variableis decorated with@provide_session. Called without a session, it goes throughcreate_session(), which returns the caller's own session for a scoped session and commits it on exit. So any code that reads a Variable while holding a transaction gets that transaction committed underneath it. That surfaces two ways:prohibit_commitguard: it raisesRuntimeError: UNEXPECTED COMMIT, which is then swallowed per-backend and surfaces as a missing Variable.Variable.get,Variable.get_variable_from_secrets, andVariable.setdefaultnow take an optional keyword-only session, forwarded only toMetastoreBackend.Variable.updateforwards its own.Affected today:
VariableIntervalreturn "Variable not found" despite the Variable existingnext_dagrun_infoleavenext_dagrunNULL, so the Dag is never eligible and never runs, with nothing loggedBoth of the above are reached from
_create_dagruns_for_dags, inside the guard. Dag sync viaupdate_dags,Variable.update(fixed here), andVariable.setdefault(fixed here) hold a session without the guard, so they fail the silent way instead.There are likely others; these are the ones I found.
Scope: This fixes the core
Variable.updateandVariable.setdefaultoutright and lets the remaining callers be fixed by passingsession.airflow.sdk.Variable.getandConnection.get_connection_from_secretsshare the same issue and are left as follow-ups, so callers going through those are still affected.Related (that I know of)
sessionthrough itsVariable.get()callcloses: #71801
Was generative AI tooling used to co-author this PR?
Generated-by: Kiro (Claude Opus 5) following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.