fix: resolve dag_run.log_template_id to target's active template during AF3 migrations - #183
sahas-kamsani wants to merge 1 commit into
Conversation
…ng AF3 migrations #181 stopped the log_template_id/backfill_id/trigger_id foreign-key violations from #179 by dropping those fields from the migration insert payload entirely. That's correct for backfill_id/trigger_id (NULL is a valid state), but not for log_template_id: Airflow has no fallback for a NULL log_template_id when resolving a task's log path, so every migrated dag_run row was left unable to serve logs for any task, on any try -- even when the worker wrote the log successfully. This traded a loud, migration-blocking FK error for a silent, deferred log-visibility gap. Resolve log_template_id to the target deployment's own currently-active log_template row instead of dropping it, mirroring how Airflow itself resolves the value via LogTemplate.latest_id() when creating a new DagRun. Resolved once per insert_directly batch and cached per-instance. Falls back to NULL (with a logged warning) if the target's log_template table is empty, rather than failing the whole batch. Updates the existing regression test from #181 (which asserted the field was stripped) to assert it's resolved to the target's id instead, and adds coverage for the once-per-batch caching and the empty-target-table fallback. Fixes #<issue-number> Related: #179, #181 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
==========================================
+ Coverage 23.07% 24.89% +1.82%
==========================================
Files 22 22
Lines 2102 2153 +51
==========================================
+ Hits 485 536 +51
Misses 1617 1617 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Tested against real Astro Cloud deployments (before/after)Verified this fix live against two Airflow-3.1.8-matched Astro Cloud deployments (source/target), Before (pre-fix 2.9.1): migrated After (this branch): migrated No regression observed in One limitation worth flagging for reviewers: I could not get an end-to-end "log now renders in the Also hit and worked around a separate, pre-existing issue while setting this up: the |
Fixes: #182
Related: #179, #181
What this changes
#181 stopped the
log_template_id/backfill_id/trigger_idforeign-key violations from #179 byremoving those fields from the migration insert payload entirely. That's correct for
backfill_id/trigger_id(safe to leave NULL — no active backfill/deferral is a valid state), butnot for
log_template_id: Airflow has no fallback for a NULLlog_template_idwhen resolving atask's log path, so every migrated
dag_runrow was left unable to serve logs for any task, on anytry, even when the worker wrote the log successfully. #181 traded a loud, migration-blocking error
for a silent, deferred one — worse UX, just delayed.
This PR resolves
log_template_idto the target deployment's own currently-activelog_templaterow instead of leaving it unset, mirroring how Airflow itself resolves the value via
LogTemplate.latest_id()when creating a newDagRun.Only
astronomer_starship/_af3/starship_compatability.pyneeds changing._af2doesn't sendlog_template_idin the first place — itsget_dag_runsprojects through the attrs-filtereddag_run_attrs(), which never listslog_template_id, whereas AF3'sget_dag_runsdoes a rawselect(table)column dump that picks up every reflected column. So this bug — and this fix — isAF3-source → AF3-target specific.
Patch
Design decisions
SELECT id FROM log_template ORDER BY id DESC LIMIT 1against thetarget's own session (
self.session) — this is exactlyMAX(id), matching Airflow's ownLogTemplate.latest_id(). Never trusts the source's id — id spaces aren't portable acrossdeployments (a real customer migration hit exactly this pitfall during manual backfill).
since the target's active template can't change mid-migration. Covered by
test_dag_run_direct_insert_resolves_log_template_id_once_per_batch.log_templatetable has zero rows (shouldn't happen in practice — Airflow seeds oneon first scheduler start): resolves to
None/NULL rather than raising, so this one edge casedoesn't abort an entire migration batch — but logs a
logger.warningso it's visible inmigration output rather than silent, which is the exact complaint this PR exists to fix. Open to
feedback if maintainers would rather this fail loudly instead — flagging it explicitly since it
was called out as an open question in earlier drafts of this fix.
Testing
tests/af3_insert_sanitization_test.py::test_dag_run_direct_insert_resolves_log_template_id_to_target_active_template— replaces the old "strips" regression test added in Strip dag_run log_template_id during AF3 migrations #181; asserts the source's id (3) does
not survive and the target's active id (2) is what actually gets inserted.
..._resolves_log_template_id_once_per_batch— 5-row batch, asserts exactly 1 lookup query...._leaves_log_template_id_null_when_target_has_none— empty-table edge case: asserts no crashand asserts the warning is logged (via
caplog).backfill_id/trigger_idstripping tests (also from Strip dag_run log_template_id during AF3 migrations #181) are unchanged and stillpass, confirming no regression there.
Verified locally:
pytest tests/af3_insert_sanitization_test.py -v→ 5/5 passed, against a realapache-airflow==3.0.6install (not mocked out) and the repo's pinnedruff==0.14.5(ruff checkruff format --checkboth clean). Fullpytest -c pyproject.tomlrun with no other regressions(the one unrelated failure, a
just build-backendshell-out invalidation_test.py, is a sandboxlimitation — no
justbinary available there — not related to this change).Not yet covered: a live end-to-end migration through the UI/API against two real Airflow 3
instances. See the companion doc
starship-local-testing-guide.mdfor a walkthrough using thisrepo's own
dev3/tooling — worth running once before merge.