ferruzzi commented on PR #70370:
URL: https://github.com/apache/airflow/pull/70370#issuecomment-5671060975
@hkc-8010 - Thanks for your patience on this one. There are a bunch of PRs
open which all edit the same portion of code. I've been trying to get them
merged in the order that creates the least number of messy merge conflicts, and
you are up next (after I get #72651 merged). It's important that you do this
merge right since the previous one in the chain added `session` to this block
and it's important we don't drop that in the rebase here.
`serialization/definitions/dag.py:763-766` on main now has:
```
interval = deserialized_deadline_alert.interval
if isinstance(interval, SerializedVariableInterval):
interval = interval.resolve(session=session)
```
Passing `session=session` there is a critical bug fix* despite it being an
optional parameter in the call. It would be easy to miss on your rebase, and
nothing would necessarily tell you. You've dropped that line in favor of the
new helper, so we have to make sure you carry the fix forward in yours.
# TLDR
Here's my proposal for your fix: Add `Session` to the `if TYPE_CHECKING`
import block then:
```
def resolve_deadline_alert_interval(
alert: SerializedDeadlineAlert, *, session: Session | None = None
) -> datetime.timedelta:
if isinstance(alert.interval, SerializedVariableInterval):
return alert.interval.resolve(session=session)
return alert.interval
```
#71968 added `test_resolve_forwards_session` but it only checks `resolve()`
itself, not what helpers are using, so nothing is protecting your particular
change. Maybe have a look at how that test works and see if you can think of a
test that would guarantee a provided session gets passed through and is the one
that is used (as opposed to creating a new session), that would be outstanding.
## * The technical reason, if you care:
This code runs under the the scheduler's `prohibit_commit` guard. When
`Variable.get` is called without a `session`, it falls through to
`@provide_session` in `MetastoreBackend.get_variable`. Since
`settings.Session` is a `scoped_session`, instead of opening a fresh `session`
it just hands back the open `session` that the scheduler is holding and tries
to commit and close it on the way out. Except (most importantly) the guard
prevents that commit and rolls back the session. You can check out
`get_variable_from_secrets` at/around `models/variable.py:502-505` if you want
to see exactly what we're trying to avoid. Either way, passing the `session`
in avoids the `provide_session` wrapper and that bug, entirely.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
