Abolfazlrwm commented on PR #76629: URL: https://github.com/apache/airflow/pull/76629#issuecomment-6711368378
Thanks for the thorough static review, @AP24110011695 — great catch on asking about downstream consumers of the reloaded instance. I traced every consumer; summary below. ### Is the substituted `dag_run` safe downstream? Yes — it never escapes validation `DagRunContext.dag_run` is typed as `ti_datamodel.DagRun` (`airflow-core/src/airflow/api_fastapi/execution_api/datamodels/taskinstance.py:376`), a pydantic `StrictBaseModel` — **not** the ORM class. The ORM object passed in `DagRun.produce_dag_callback()` (`models/dagrun.py:1776-1779`) exists as an ORM instance only *inside* the `mode="before"` validator, where pydantic immediately converts it to the datamodel (the datamodel's own `safe_extract_from_orm` before-validator handles the field-by-field extraction). So: 1. **No downstream consumer ever sees an ORM instance.** `_execute_dag_callbacks` (`dag_processing/processor.py:416`) puts `ctx_from_server.dag_run` — already the datamodel — into `TIRunContext.model_construct(...)` and `runtime_ti.get_template_context()`, reading only datamodel column fields (`run_id`, dates, `state`, ...). The `dag.test` path (`execute_dag_callbacks`, `models/dagrun.py:1813`) uses `self.dag_run_data` and never constructs a `DagRunContext`, so it is untouched by this change. 2. **The reload eager-loads exactly the only relationships the conversion can touch.** The sole relationship field on the datamodel is `consumed_asset_events` (`taskinstance.py:396`), whose items carry nested `asset` and `source_aliases` (`taskinstance.py:365-371`) — precisely the three `selectinload`s in the recovery query. Every other datamodel field is a column attribute, loaded with the row. 3. **No regression surface.** On the recovery path, the alternative to the substitution is the original broken instance, where even *column* access failed (that is the bug) — so there is no case where the substitution changes behavior that previously worked. Your point about the reloaded instance being detached-but-fully-loaded after `create_session()` closes is exactly why the eager loads are there: nothing lazily hits a session afterwards. ### CI failures are unrelated (known flaky, already fixed on main) The 4 failed checks all trace to a single test: `TestFileTaskLogHandler::test_read_remote_logs_with_real_s3_remote_log_io` (`tests/unit/utils/test_log_handlers.py:851`), failing with the identical `assert logs[0] == log_content` assertion on sqlite, postgres **and** mysql. Nothing in this change touches task-log handling. That test was fixed on main by #76632, which merged ~3h after this PR's commit — i.e. after the branch point. I've updated the branch with main via "Update branch", which picks that fix up, so the fresh CI run should be green modulo unrelated flakes. I also re-ran the full `tests/unit/callbacks/test_callback_requests.py` locally on Python 3.12 + sqlite against the PR head: **22/22 passed**, including the two new recovery-path tests. -- 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]
