Conversation
MetastoreBackend.get_connection() and get_variable() are decorated
with @provide_session. Called with no session -- the shape every
public caller uses, including BaseHook.get_connection(conn_id) and
Variable.get(key) from a dag_run listener hook
(on_dag_run_running/success/failed) -- the decorator resolves to
settings.Session(), a thread-local scoped session.
The scheduler's own _do_scheduling loop runs dag_run listener hooks
in-process while holding that exact same scoped session under
prohibit_commit, a guard that raises on any unexpected commit to
protect its HA locking. The @provide_session wrapper commits on exit,
so a listener that reads a Connection or Variable trips the guard with
RuntimeError("UNEXPECTED COMMIT - THIS WILL BREAK HA LOCKS!"), which
is swallowed per-backend and surfaces first as a spurious
AirflowNotFoundException, then corrupts the scheduler's session for
the rest of that scheduling pass -- observed cascading into
DetachedInstanceError on unrelated DagRun objects in the same batch.
Give get_connection()/get_variable() a genuinely independent,
non-scoped session (create_session(scoped=False)) when no session is
passed, so a listener's read no longer aliases into -- and commits --
the scheduler's locked session.
closes: apache#39646
Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
Generated-by: Claude Code (Sonnet 5)
Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com> Generated-by: Claude Code (Sonnet 5)
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_connection()andget_variable()are decorated with@provide_session. Called with no session — the shape every public caller uses, includingBaseHook.get_connection(conn_id)andVariable.get(key)from adag_runlistener hook (on_dag_run_running/on_dag_run_success/on_dag_run_failed) — the decorator resolves the session tosettings.Session(), a thread-local scoped session.The scheduler's own
_do_schedulingloop runsdag_runlistener hooks in-process (DagRun.notify_dagrun_state_changed) while holding that exact same scoped session underprohibit_commit, a guard that raises on any unexpected commit to protect its HA locking. The@provide_sessionwrapper commits on exit, so a listener that reads a Connection or Variable trips the guard withRuntimeError("UNEXPECTED COMMIT - THIS WILL BREAK HA LOCKS!"). That error is swallowed per-backend insideConnection.get_connection_from_secrets/Variable.get_variable_from_secrets, so it first surfaces as a spuriousAirflowNotFoundExceptionfor a Connection/Variable that genuinely exists — and then corrupts the scheduler's session for the rest of that scheduling pass, observed cascading intoDetachedInstanceErroron unrelatedDagRunobjects processed in the same batch.This is the root cause of #39646: a listener plugin calling
BaseHook.get_connection()/Variable.get()fromon_dag_run_running/on_dag_run_success/on_dag_run_failedgetsAirflowNotFoundExceptionfor connections/variables that are defined, while the identical call fromon_task_instance_*(which runs in a separate worker process, not the scheduler) works fine.Impact: crash-on-valid-input
WHO reaches this / entry point: any dag_run listener plugin registered the documented way (a
hookimplimplementingon_dag_run_running/on_dag_run_success/on_dag_run_failed, exactly as shown in the Listeners how-to doc and in #39646's own repro) that calls the standardBaseHook.get_connection(conn_id)orVariable.get(key)— no session argument, which is the only shape a plugin author has, since the hookspec passes no session. Triggered by: the scheduler reachingSchedulerJobRunner._do_scheduling->_schedule_all_dag_runs/_start_queued_dagruns->DagRun.notify_dagrun_state_changedfor any DagRun transitioning to running/success/failed while such a listener is registered -- an entirely ordinary scheduling pass, valid input by construction (a real, already-committed Connection/Variable). What they observe: first a wrongAirflowNotFoundExceptionfor a Connection/Variable that exists, then (per the DetachedInstanceError repro below) the scheduler's own session breaks for the rest of that scheduling pass.Fix
Give
get_connection()/get_variable()a genuinely independent, non-scoped session (create_session(scoped=False)) when no session is passed, instead of the scoped one@provide_session's default would resolve to. A listener's read no longer aliases into — and commits — the scheduler's locked session.@provide_sessionitself can't be reused for this (itsNEW_SESSIONpath is exactly the scoped session that's the problem), so the session handling is inlined; thecheck-new-session-in-provide-sessionprek hook's None-typed exemption (session: Session | None = None) covers the new shape.This is a narrower, session-less-caller-focused companion to #71968/#72121, which added an optional session-reuse parameter for internal scheduler call sites that already have a session in scope (
Variable.get_variable_from_secrets). Neither of those coversBaseHook.get_connection()/Variable.get()called with no session at all, which is the public, session-agnostic shape every dag_run listener (and every non-scheduler caller) actually uses — #71968's own body namesConnection.get_connection_from_secretsas exactly this left-open follow-up.Testing
Two new tests in
TestMetastoreBackendSessionSafety(tests/unit/always/test_secrets_metastore.py), mirroring theprohibit_commit-under-guard pattern from #67980'stask_instance_mutation_hookregression tests:test_get_connection_survives_prohibit_commit_without_explicit_sessiontest_get_variable_survives_prohibit_commit_without_explicit_sessionBoth reproduce the exact scheduler shape (
with create_session() as session: with prohibit_commit(session): MetastoreBackend().get_connection(...)/get_variable(...), nosession=argument) and assert the lookup succeeds.Negative control: reverting only
metastore.pytomainand running the class (pytest tests/unit/always/test_secrets_metastore.py::TestMetastoreBackendSessionSafety):With the fix, all 7 tests in the class pass (the 5 pre-existing tests plus the 2 new ones),
7 passed. Also re-verified the original issue's exact scenario end to end against a realSchedulerJobRunner._do_scheduling()run with a registeredon_dag_run_successlistener callingBaseHook.get_connection()/Variable.get()on a real, committed Connection/Variable: fails withDetachedInstanceErroron pristinemain, passes with this fix.closes: #39646
Signed-off-by: Udaya Tejas udayatejas2004@gmail.com
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Sonnet 5) following the guidelines