Skip to content

Commit 740855a

Browse files
vatsrahul1001potiuk
andcommitted
Don't crash supervisor IPC loop on transient network errors (#66572) (#67177)
* Don't crash supervisor IPC loop on transient network errors handle_requests in the supervisor only caught ServerResponseError. Any non-HTTP exception (httpx.ConnectError, httpx.TimeoutException, socket timeouts, etc.) would propagate, terminate the generator, and permanently break the supervisor-to-task IPC channel. The task subprocess would then get EOFError on every subsequent send, and the worker would be stuck waiting for replies that never come. Add a catch-all except Exception after the ServerResponseError handler that logs the unhandled exception with type info, sends a best-effort ErrorResponse(API_SERVER_ERROR, ...) back to the task so the failure surfaces in task logs (wrapped in suppress(Exception) because if we can't reach the task subprocess via stdin we shouldn't double-fault), and lets the request loop continue to the next request. Test added: a fake httpx.ConnectError on the first call produces an ErrorResponse, the generator stays alive, and a second request is processed normally (the loop is not dead). Reported by the L3 ASVS sweep at apache/tooling-agents#24 (FINDING-005). * Address review comments: shorten comment and use exc_info - Shorten the catch-all comment per amoghrajesh's suggestion. - Use exc_info=e in log.exception instead of exception_type field per jason810496's suggestion (exception type is redundant since the exception itself is logged with full type info and traceback). (cherry picked from commit 1e5d799) Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
1 parent 3a734dd commit 740855a

2 files changed

Lines changed: 62 additions & 0 deletions

File tree

task-sdk/src/airflow/sdk/execution_time/supervisor.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -760,6 +760,28 @@ def handle_requests(self, log: FilteringBoundLogger) -> Generator[None, _Request
760760
),
761761
request_id=request.id,
762762
)
763+
except Exception as e:
764+
# Generic exception handling so a transient network error (httpx.ConnectError /
765+
# httpx.TimeoutException) or any other exception
766+
# doesn't crash this generator and crash the IPC communication between supervisor and task.
767+
log.exception(
768+
"Unhandled exception while handling task request",
769+
request_id=request.id,
770+
exc_info=e,
771+
)
772+
with suppress(Exception):
773+
self.send_msg(
774+
msg=None,
775+
error=ErrorResponse(
776+
error=ErrorType.API_SERVER_ERROR,
777+
detail={
778+
"status_code": None,
779+
"message": str(e),
780+
"exception_type": type(e).__name__,
781+
},
782+
),
783+
request_id=request.id,
784+
)
763785

764786
def _handle_request(self, msg, log: FilteringBoundLogger, req_id: int) -> None:
765787
raise NotImplementedError()

task-sdk/tests/task_sdk/execution_time/test_supervisor.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2753,6 +2753,46 @@ def test_handle_requests_api_server_error(self, watched_subprocess, mocker):
27532753
"detail": error.response.json(),
27542754
}
27552755

2756+
def test_handle_requests_network_exception_does_not_crash_loop(self, watched_subprocess, mocker):
2757+
"""A transient network error must not crash the IPC generator.
2758+
2759+
Without the catch-all in handle_requests, an httpx.ConnectError would
2760+
propagate, the generator would terminate, the task subprocess would
2761+
get EOFError on every subsequent send, and the worker would be stuck.
2762+
Verify that the error is reported back to the task as an
2763+
API_SERVER_ERROR ErrorResponse and that the loop stays alive for the
2764+
next request.
2765+
"""
2766+
watched_subprocess, read_socket = watched_subprocess
2767+
2768+
# First request raises a network exception, second succeeds.
2769+
first_call = httpx.ConnectError("connection refused")
2770+
watched_subprocess.client.task_instances.succeed = mocker.Mock(side_effect=[first_call, None])
2771+
2772+
generator = watched_subprocess.handle_requests(log=mocker.Mock())
2773+
next(generator)
2774+
2775+
# First request — should produce an ErrorResponse, not crash the generator.
2776+
msg1 = SucceedTask(end_date=timezone.parse("2024-10-31T12:00:00Z"))
2777+
req1 = _RequestFrame(id=randint(1, 2**32 - 1), body=msg1.model_dump())
2778+
generator.send(req1)
2779+
2780+
read_socket.settimeout(0.5)
2781+
frame_len = int.from_bytes(read_socket.recv(4), "big")
2782+
bytes_ = read_socket.recv(frame_len)
2783+
frame = msgspec.msgpack.Decoder(_ResponseFrame).decode(bytes_)
2784+
2785+
assert frame.id == req1.id
2786+
assert frame.error is not None
2787+
assert frame.error["error"] == "API_SERVER_ERROR"
2788+
assert frame.error["detail"]["exception_type"] == "ConnectError"
2789+
2790+
# Second request — generator must still be alive and process it normally.
2791+
msg2 = SucceedTask(end_date=timezone.parse("2024-10-31T12:01:00Z"))
2792+
req2 = _RequestFrame(id=randint(1, 2**32 - 1), body=msg2.model_dump())
2793+
# Should not raise StopIteration (which would mean the loop crashed).
2794+
generator.send(req2)
2795+
27562796

27572797
class TestSetSupervisorComms:
27582798
class DummyComms:

0 commit comments

Comments
 (0)