Uh oh!
There was an error while loading. Please reload this page.
Allow Variable.get to reuse the caller's database session - #71968
Allow Variable.get to reuse the caller's database session#71968ferruzzi 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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ferruzzi
left a comment
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.
| :param session: Existing session to reuse for the metadata database read and write. | ||
| Callers holding an open transaction must pass it. |
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.
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.