Fix mismatched XCom values when a dynamically mapped task has a gap - #72007
Open
ColtenOuO wants to merge 3 commits into
Open
Fix mismatched XCom values when a dynamically mapped task has a gap#72007ColtenOuO wants to merge 3 commits into
ColtenOuO wants to merge 3 commits into
Conversation
Reading XCom values for a mapped task or task group by sequence index or slice treated the position among existing XCom rows as the map_index. When a mapped task instance hasn't pushed an XCom yet (still up_for_reschedule, or skipped), the gap silently shifted every later map_index's value down by one, so downstream tasks could read the wrong value or hit an IndexError even though the upstream task instances existed.
The sparse-index fix for slice reads unconditionally fetched all of a mapped task's XCom rows into memory to place each value at its true map_index. That regressed the common case (no gap) versus the previous SQL-side pagination, and XCom payloads have no size cap -- only the number of mapped instances does. Only fall into the full-fetch path once a gap is actually detected via a COUNT query.
The sparse-index handling only checked whether the task was mapped, not whether the requested key had ever been pushed at all. Querying a wrong or nonexistent key against a mapped task incorrectly took the sparse path and returned every offset as null / the full-length array filled with null, instead of 404 / an empty list.
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.
Sumarry
Fix the Execution API's XCom sequence-read endpoints so a mapped task/task-group instance that hasn't pushed an XCom yet (still
up_for_reschedule, or skipped) doesn't shift the values of every mapped instance after it.closes: #40321
The bug
get_mapped_xcom_by_index,get_mapped_xcom_by_slice, andhead_xcom(inairflow-core/src/airflow/api_fastapi/execution_api/routes/xcoms.py) all built their result by querying XCom rows ordered bymap_indexand then using the position in that result set as if it were themap_indexitself (via SQLOFFSET/LIMIT/slice(), withmap_indexdropped from the response).That's correct only when every mapped instance has already pushed an XCom. If one hasn't — e.g. a sensor inside a dynamically mapped
@task_groupis stillup_for_reschedule— there's no row for itsmap_index, so the query silently returns one fewer row and every latermap_index's value shifts down by one position. A downstream task readingmapped_group.output[i](or iterating over it) can get another instance's value instead of the one it asked for, or hit anIndexErrorreading past the (now short) end.This is the same root cause reported in #40321 in 2024 against the Airflow 2.x
LazyXComSelectSequenceimplementation. The AIP-72 rewrite reimplemented the same "position == map_index" assumption in the new task-sdk/execution-API code path instead of fixing it, so it's still present onmaintoday.Confirming the bug is still there, and that the fix resolves it
Before writing the fix, I reproduced the bug against current
mainwith a test scenario matching the issue: a task mapped over 4 values, wheremap_index=1never gets an XCom row (simulating a mapped instance that's stillup_for_rescheduleand hasn't pushed anything). Querying the slice endpoint for all 4 values returned only 3 —["f", "o", "b"]— with the value that actually belongs tomap_index=3silently shifted intomap_index=1's position. TheHEADendpoint's count also undercounted (3 instead of 4). This matches exactly what the issue reports: values read by a downstream task line up with the wrongmap_index, and the reported length is wrong too.Two of the existing tests in
test_xcoms.py(test_xcom_get_with_offset,test_xcom_get_with_slice) already built this exact gapped scenario, but asserted the compacted 3-item result as the expected outcome — meaning the buggy behavior was pinned as correct by the test suite rather than being an untested gap.After the fix, I reran the same scenario: the slice endpoint now returns the full 4-item
["f", None, "o", "b"], withmap_index=3's value correctly in position 3, andHEAD's count correctly reports 4. I updated the two pinning tests to assert this corrected behavior and addedtest_xcom_get_with_slice_and_count_unfinished_mapped_taskas an explicit regression test for the issue's scenario (a task instance that never pushed an XCom at all, as opposed to one whose pushed value happened to beNone— those are different situations that both need to resolve correctly).The fix
Added
_get_mapped_length(), which resolves the logical number of mapped instances for(dag_id, run_id, task_id)from theTaskInstancetable (every mapped instance gets a row at expansion time, whether or not it has run yet — seeTaskMap.expand_mapped_task), independent of how many XCom rows exist.When a task is mapped in this run, the three endpoints now key results by the actual
map_indexinstead of result position, filling inNonefor anymap_indexthat has no XCom row yet:get_mapped_xcom_by_index:offsetresolves against the logical0..mapped_length-1range; a valid-but-missing index returnsNone(200), not a 404 — only an out-of-range index 404s.get_mapped_xcom_by_slice: builds amap_index -> valuedict, fills gaps withNone, then applies the slice with plain Python slicing.head_xcom: reports the logical mapped length inContent-Range, not the XCom row count.Two cases are deliberately left on the old (pre-fix) behavior, since there's no well-defined "logical length" for them:
include_prior_dates=Trueon the slice endpoint can span multiple dag runs, each with its own (potentially different) mapped length.map_index=-1, no expansion) has no mapped-length concept at all — it falls through to the original row-position logic unchanged.get_xcom'soffsetquery-parameter branch (a separate, older code path) is intentionally untouched — no task-sdk client currently calls it withoffsetset, and it has its own pre-existing "skip None values" semantics that aren't part of this bug.Blast radius: what else calls these endpoints, and what changes for callers
I checked who actually consumes these three routes and what the response-shape change means for them, since this touches a fairly central part of the Execution API.
Consumers.
task-sdk'sLazyXComSequence(task-sdk/src/airflow/sdk/execution_time/lazy_sequence.py) is the only real consumer, reached throughXComOperationsintask-sdk/src/airflow/sdk/api/client.py. I grepped the rest of the repo (providers/, the UI, everywhere else) for these route paths and the relevant symbol names and found no other caller — nothing outside task-sdk talks to these three endpoints directly.client.pyitself is a thin pass-through and doesn't assume anything about the old semantics, so it needed no changes.User-visible behavior change. This is the one worth calling out explicitly: a mapped task's
.outputused to silently compact away gaps when iterated, sliced, or expanded over downstream (for v in t.output,.expand(x=t.output), etc.) — a missing map_index just meant the list was shorter than the actual mapped count, with everything after it shifted into the wrong position. After this fix,.outputis the correct length and gaps show up explicitly asNoneat their true position. This is the intended correctness fix, but it's a behavior change any DAG author relying (knowingly or not) on the old compaction would notice — e.g..expand(x=t.output)now creates an instance withx=Nonefor the gap instead of one fewer instance. Flagging this for review since it's a semantic change, not just a bugfix that's invisible from the outside.Performance. The original version of this fix always fetched every XCom row for the task into a Python dict once a task was detected as mapped, even when there was no gap at all — regressing the existing SQL-side
OFFSET/LIMIT/slice()pagination for the common case. Since XCom values are stored in an unbounded JSON column (only the count of mapped instances is capped, bycore.max_map_length, default 1024), that could mean loading a large number of large payloads into API-server memory for a single-item slice request. Fixed by adding one cheapCOUNTquery to detect whether a gap actually exists before falling into the full-fetch path — the common case (no gaps) keeps the original SQL pagination; only a genuinely sparse sequence pays the cost of loading every row, which is unavoidable since placing values at their truemap_indexrequires knowing all of them. Addedtest_xcom_get_with_slice_mapped_task_without_gap_uses_sql_paginationto cover the no-gap path explicitly, since the existing gap-scenario tests wouldn't have caught a regression here.Testing
test_xcom_get_with_offset/test_xcom_get_with_sliceto assert the corrected sparse result instead of the compacted one, and extended their index/slice boundaries to cover the full logical range.test_xcom_get_with_slice_and_count_unfinished_mapped_task: the regression test described above, covering both the slice and count (HEAD) endpoints.test_xcom_get_with_slice_mapped_task_without_gap_uses_sql_pagination, covering the no-gap performance path described above.test_getitem_index_sparse_gap_returns_nonein task-sdk'stest_lazy_sequence.py, confirming the client resolves a gap toNonerather than raisingIndexError. Traced this throughXCom.deserialize_value/airflow.sdk.serde.deserialize, which already short-circuits onNone, so no task-sdk source change was needed.versions/v2025_04_28API-version contract test (a different code path, untouched by this fix) still passes unchanged.ruffandmypy(viabreeze run mypy) pass on all changed files.Was generative AI tooling used to co-author this PR?