henry3260 opened a new pull request, #76537: URL: https://github.com/apache/airflow/pull/76537
related: #76425 ## Why A collection response field annotated with `Iterable[...]` is validated by pydantic lazily, while FastAPI serializes the response on the event loop. Each item's attributes are resolved there, on the sync engine: a lazy relationship or a `selectin` post-load becomes a query that blocks the whole API server once the connection pool is busy. `PoolCollectionResponse.pools` was fixed by hand in #74247, and #76525 proposes the same fix for `DAGCollectionResponse.dags` and `DeadlineCollectionResponse.deadlines`, each time copying the same explanatory comment into another datamodel. Twenty occurrences exist across 16 files at this commit, including those two. The rest are safe only because their routes load eagerly or build the items first (`routes/public/assets.py:480-485`, `routes/public/xcom.py:309-314`) or because their response models read columns only — and nothing stops a new one from being added. `list[...]` validates on construction, inside the route, and renders to the same `type: array` in the OpenAPI schema, so there is no reason to declare one of these fields lazily. This adds a ratcheting check so the shape stops coming back while the remainder are migrated under #76425. ## What - `scripts/ci/prek/check_no_lazy_collection_response.py` — new hook. It parses each file with `ast` and reports every field of a `*Collection` / `*CollectionResponse` class whose annotation mentions `Iterable`, `Generator`, `Iterator`, `AsyncIterable` or `AsyncIterator`, so `abc.Iterable[X]` and `Iterable[X] | None` match too. Only the first two are validated lazily (the others raise `PydanticSchemaGenerationError` at class definition); all five are listed to keep the rule memorable. It subclasses the repo's `AllowlistManager`, following `check_new_airflow_exception_usage.py`, with the same `--generate` and `--cleanup` modes. - `generated/known_lazy_collection_responses.txt` — the grandfathering allowlist: 16 files / 20 occurrences, so no existing file turns CI red. A file that exceeds its recorded count fails the hook; when a count drops, the hook rewrites the entry and exits non-zero so the tightened allowlist is staged together with the fix. `generated/*` is in `.rat-excludes`, so the file carries no licence header. - `.pre-commit-config.yaml` — registers `check-no-new-lazy-collection-responses`. It runs `--all-files` with `pass_filenames: false`: the allowlist is one file for the whole tree, so a per-file run cannot see a raised limit, and parallel batches would each overwrite the others' tightenings. - `scripts/tests/ci/prek/test_check_no_lazy_collection_response.py` — 33 tests covering the detection, the ratchet, and each mode of `main`. Two details worth a reviewer's eye: - `iter_files` lists files with `git ls-files` rather than walking the directory, so an untracked work-in-progress file cannot fail someone's unrelated commit and `--generate` cannot record it as an exemption. It runs from the repository root with a literal directory pathspec because `GIT_DIR` alone, or any of the `GIT_*_PATHSPECS` variables, otherwise makes the listing empty or partial — and an empty listing is refused rather than read as a clean tree. - A file that cannot be parsed is skipped with a message and keeps its allowlist entry, so a syntax error mid-edit does not look like a legitimate drop to zero. Known limits, so reviewers can judge the scope: the check reads the annotation, not what a route does, so it does not catch the other event-loop shapes listed in #76425; a fix and a regression in the same file keep its count level; a class not named `*Collection` / `*CollectionResponse`, or one outside `airflow-core/src/airflow/api_fastapi/`, is out of scope; and `--generate` can legitimise new occurrences, so the allowlist diff is worth a look during review. A follow-up in `AllowlistManager` itself would help all four allowlist hooks: dropping entries for deleted files, and refusing `--generate` when none of the listed files exist (a sparse checkout). ### Validation - `uv run --project scripts pytest scripts/tests/ci/prek/test_check_no_lazy_collection_response.py` — 33 passed. - `prek run check-no-new-lazy-collection-responses --all-files` — passes as the tree stands, so the recorded occurrences do not turn CI red. - Declaring `PoolCollectionResponse.pools` as `Iterable[PoolResponse]` fails the hook and names the offending field. Raising a limit in the allowlist alone also fails, and the hook tightens it back. Changing `DAGCollectionResponse.dags` to `list[...]` removes its allowlist entry and exits non-zero asking for the file to be staged. All edits reverted. If #76525 merges first, I will rebase and re-run `--generate` so the allowlist starts out accurate for `datamodels/dags.py` and `datamodels/ui/deadline.py`. --- ##### Was generative AI tooling used to co-author this PR? - [x] Yes — Claude Code (Claude Opus 5) Generated-by: Claude Code (Claude Opus 5) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions) 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
