Skip to content

Fix RecursionError in common.compat hook lineage add_extra polyfill#68735

Merged
kaxil merged 1 commit into
apache:mainfrom
astronomer:fix-compat-lineage-collector-recursion
Jun 19, 2026
Merged

Fix RecursionError in common.compat hook lineage add_extra polyfill#68735
kaxil merged 1 commit into
apache:mainfrom
astronomer:fix-compat-lineage-collector-recursion

Conversation

@kaxil

@kaxil kaxil commented Jun 19, 2026

Copy link
Copy Markdown
Member

Summary

On Airflow < 3.2, common.compat's get_hook_lineage_collector() polyfills add_extra by
monkeypatching the collected_assets and has_collected class properties of the lineage
collector. _add_extra_polyfill was not idempotent: the trigger gate (_lacks_add_extra_method)
checks instance-level _extra, so a fresh collector instance of an already-patched class
re-enters the polyfill and re-wraps the class property, capturing the previously installed wrapper
as the "original". Each call stacks another layer onto the getter chain, so once enough collectors
are created the next collected_assets / has_collected access exceeds the recursion limit and
raises RecursionError.

Where it shows up

In production the global collector is a process singleton (the accessor is @cache-decorated) and
the polyfill only runs on Airflow < 3.2, so it is applied exactly once -- no impact. The failure
surfaces on the Compat 3.1.x provider test matrix, where a fresh HookLineageCollector is built
per test: the class accumulates one wrapper per test until the lineage edge-case tests blow the
recursion limit. This is a test-suite/robustness fix, not a production incident.

Fix

Capture the collector's true original collected_assets / has_collected getters once per class
(stashed in the class's own __dict__) and always wrap that original, rather than whatever getter is
currently installed:

  • The polyfill is re-applied for every fresh collector instance, and on Airflow 2 the asset-naming
    layer re-patches collected_assets before each call. Wrapping the current value would capture the
    previous compat wrapper as the "original", so the getter chain grew one layer per call until access
    hit the recursion limit.
  • Wrapping the stored original instead keeps the chain flat (depth one) no matter how many times the
    polyfill runs, while still re-applying the extra wrapper each call -- so it composes correctly
    with the Airflow 2 asset-naming layer that re-patches collected_assets.
  • The stash is keyed on the class's own __dict__, so a subclass that overrides these properties is
    captured in its own right rather than reusing a base class's original.
  • Instance _extra / _extra_counts are initialized only when missing, so re-applying never clears a
    collector that already accumulated extras.

Behavior is otherwise unchanged: every collector still gets add_extra and the extended
collected_assets / has_collected.

@kaxil
kaxil requested review from kacpermuda and mobuchowski June 19, 2026 00:31
@kaxil kaxil added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jun 19, 2026
@kacpermuda

Copy link
Copy Markdown
Collaborator

Thanks Kaxil for fixing this, missed that for sure when writing tests

_add_extra_polyfill re-patched the collector's collected_assets/has_collected class properties on every call (the trigger gate keys on instance-level _extra), stacking a wrapper layer per fresh collector until property access raised RecursionError on the Compat 3.1.x provider test matrix. Patch each class exactly once via a per-class marker set after all patches; production is unaffected (singleton collector).
@kaxil
kaxil force-pushed the fix-compat-lineage-collector-recursion branch from 0508b97 to 7a07bce Compare June 19, 2026 09:43
@kaxil
kaxil merged commit d445240 into apache:main Jun 19, 2026
91 checks passed
@kaxil
kaxil deleted the fix-compat-lineage-collector-recursion branch June 19, 2026 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:common-compat

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants