Repository navigation
Conversation
rjgoyln
changed the title
potiuk
added
the
ready for maintainer review
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Approving — solid fix, and the tests are better than most bug fixes get: parametrizing the media-type variants and pinning application/jsonp as a negative is exactly the trap a looser startswith check would have fallen into.
I went looking for two ways this could have been incomplete, and both came back clean:
- Widening the media-type match in the Task SDK without adding the decode guard there. Not a problem —
from_responsealready wraps parsing intry/exceptwith anmsgspecfallback and returnsNone(task-sdk/src/airflow/sdk/api/client.py:1372-1377). The explicit guard went into the one place that genuinely lacked one. - Passing a server-supplied dict straight into
extra=. Safe here, but worth saying why I checked:airflowctl/api/client.py:82isstructlog.get_logger()and nothing in the distribution callsstructlog.configure(), soextrais just an event key. If that logger were routed through stdliblogging, a proxy error page like{"message": "Bad Gateway"}would still raiseKeyError: "Attempt to overwrite 'message' in LogRecord"— the same class of bug, from the same source. Something to keep in mind if airflowctl's logging setup ever changes.
One nit inline, and two things before this can land.
Please add a newsfragment
airflow-core/newsfragments/70936.bugfix.rst — the diff touches task-sdk/, and those changes ship in airflow-core. Both effects are user-visible: airflowctl printing a raw httpx.HTTPStatusError traceback instead of the server's message, and the supervisor / task runner's status-specific handling silently not matching behind a proxy. Defaulting to omitting one was the right call on your side — this is the maintainer asking for it.
Please rebase before this merges
The branch is 996 commits behind main and the last commit is from 2026-08-06, so the green CI on this PR was run against a tree that has moved a long way since. I'm approving on the strength of reading the diff, not on that run — please rebase (or use the update-branch button) so real CI executes against current main. I'll merge once it comes back green; the newsfragment can ride along in the same push.
This review was drafted by an AI-assisted tool and confirmed by an Airflow maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.
More on how Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
airflowctl compared the error response content-type against the exact string "application/json". Any reverse proxy, ingress, or API gateway that returns "application/json; charset=utf-8" — or emits its own "application/problem+json" error — fell outside that comparison, so ServerResponseError was never constructed and the friendly "not found" messages in the dags and tasks commands were skipped. The bare httpx.HTTPStatusError raised instead is not handled by safe_call_command, so users saw a raw traceback. Airflow's own API server does not send a charset, which is why this only surfaces once something sits in front of it. Media types are case-insensitive and may carry parameters (RFC 9110), so the header has to be parsed rather than compared whole. Matching more responses also routes bodies into the error path that are valid JSON but not objects, which dict() rejected with a ValueError from that same unprotected path, and bodies that do not decode at all.
The Task SDK client carries the identical exact-string content-type comparison, so a task talking to the API server through a proxy that declares a charset loses the server's error detail and, because the raised exception is then a plain httpx.HTTPStatusError, no longer hits the status-specific handling in the supervisor and the task runner.
The airflowctl and Task SDK copies are the same three lines with different comments, and they already drifted in the commits that created them. A pointer in each makes the next person notice there are two.
Requested during review: the Task SDK half of the change ships in airflow-core, and both effects are visible to users running behind a proxy.
ae1b2eb to
ab5f552
Compare
potiuk
added
the
closed because of open PR limit
|
Hello @rjgoyln - thank you for your contributions to Apache Airflow! The Airflow community has introduced a limit of 5 open pull requests at a time for contributors without write access to the repository. You currently have 24 open pull requests, so - as a one-time step of introducing the limit - we closed the ones where maintainers have not engaged yet:
These pull requests stay open because maintainers are already engaged in them - they count towards your limit:
This is not a judgement of you or of your changes. We never told contributors before that opening many pull requests at once was a problem, so there is nothing to feel bad about - and nothing is lost: your branches, commits and the review history stay where they are. What we ask you to do is to make your first prioritization decision: choose which of the pull requests above matter most to you, and reopen them (up to 5 open at a time, including the ones still open) with the "Reopen pull request" button or While your pull requests are waiting for review, the most valuable thing you can do is help in other ways - reviewing other contributors' pull requests, helping with issues, and taking part in the discussions on the devlist and Slack. Why we introduced the limit, what it means for you and how to reopen or restore a pull request is explained in https://github.com/apache/airflow/blob/main/contributing-docs/32_open_pull_request_limit.rst. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
3 participants
Summary
Both API clients recognised an error body only when the response
content-typewas exactlyapplication/json, so a proxy that rewrites the header toapplication/json; charset=utf-8skipped error parsing altogether:airflowctlprinted a rawhttpx.HTTPStatusErrortraceback in place of the server's message, and in the Task SDK the exception was no longer aServerResponseError, so the status-specific handling in the supervisor and the task runner stopped matching. Airflow's own API server sends the bare media type, so this needs something sitting between client and server.Change
Two adjacent airflowctl crashes go with it: a non-object JSON error body made
log.warning(extra=...)raise, and a body labelled JSON that does not decode raisedValueErrorinstead of falling through toraise_for_status().Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines