Repository navigation
Fix Dag processor API client not catching Task SDK client errors - #74728
ephraimbuddy wants to merge 3 commits into
Conversation
The Task SDK Client moved from httpx to httpx2 in apache#73116, and httpx2's exception and transport classes are separate from httpx's. DagProcessorAPIClient subclasses that Client but still caught httpx exceptions, so registration conflicts, closed-Job retirement and early token renewal fallbacks never matched the errors the client actually raises, and its overrides were typed against the wrong Response, Timeout and Headers classes. The rest of airflow-core stays on httpx until the wider migration tracked in apache#74244 and apache#70522.
ServerResponseError.from_response compared the Content-Type header for exact equality with application/json, so a JSON error sent as "application/json; charset=utf-8", or in different letter case, was raised as a bare HTTPStatusError with its body unread. Callers that inspect the error detail, such as the Dag processor client deciding whether its registration was retired or its Job closed, then found no reason and treated the response as an unexplained failure.
The Dag processor API client kept catching httpx exceptions after the Task SDK client moved to httpx2, and nothing at lint time flagged the mismatch. With httpx banned, the next mix-up is a ruff error instead of failing tests; the modules not yet migrated are exempt until apache#70522 moves them.
boring-cyborg
Bot
added
area:API
The Task SDK client raises httpx2 exceptions and requires matching transport and timeout objects. Integrate apache#74728 so isolated processors can recover from API failures and retain structured error reasons, including in the manager and job runner paths introduced by this branch.
| from uuid import UUID | ||
|
|
||
| import httpx | ||
| import httpx2 |
There was a problem hiding this comment.
This file and both test modules are already on main through #74731, which merged after this branch was last pushed. git diff between this head and main is empty for all three. After a rebase, what's left is the from_response media-type change, its test, and the ruff ban. Could you retitle around those? As it stands the squash commit would claim the #74731 fix a second time in the changelog, and the description's "fails 86 of 98 tests on main" no longer describes main.
| return None | ||
|
|
||
| if response.headers.get("content-type") != "application/json": | ||
| media_type = response.headers.get("content-type", "").partition(";")[0].strip().lower() |
There was a problem hiding this comment.
Which server sends application/json; charset=utf-8 here? FastAPI's JSONResponse sets plain application/json (Starlette only appends a charset for text/*), so I think this branch only fires behind a proxy that rewrites the content type. That's reasonable to tolerate, but it reaches every SDK client, not just the processor: a proxy-generated JSON 404/409/410 on a heartbeat now becomes a ServerResponseError and takes the immediate SIGTERM path in supervisor.py instead of going through _handle_heartbeat_failures. Could the description say that, rather than framing it as a processor-client fix?
|
closing as #74731 was merged ahead of this |
DagProcessorAPIClient(#74370) subclasses the Task SDKClient, which #73116 moved fromhttpxtohttpx2. httpx2 has its own exception and transport classes, so the processor client'sexcept httpx.HTTPStatusError/httpx.HTTPError/httpx.ResponseNotReadclauses stopped matching anything the client raises. Registration conflicts, closed-Job retirement and the early token-renewal fallbacks all fell through. On main,dag_processing/test_api_client.pyandexecution_api/test_dag_processor_client.pyfail 86 of 98 tests, becausehttpx.MockTransportresponses trip httpx2'sSyncByteStreamassertion.httpx2. The test changes are the mechanicalhttpx→httpx2swap; no assertions changed.ServerResponseError.from_responseaccept JSON errors whoseContent-Typecarries parameters (application/json; charset=utf-8) or different letter case. Those were raised as a bareHTTPStatusErrorwith the body unread, so the processor client could not see the errorreason.httpximports in airflow-core with ruff (TID251), as task-sdk already does, so the next mix-up is a lint error instead of failing tests. The six modules not yet migrated are exempt until Consider swappinghttpxusage forhttpx2#70522 moves them (wider migration also in Migrate airflow-core tohttpx2#74244).related: #70522
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5.5) following the guidelines