From fefc3754b392e955ff487b4e544e3ef76614034e Mon Sep 17 00:00:00 2001 From: Karthic Raghupathi Date: Wed, 24 Jun 2026 22:09:43 -0400 Subject: [PATCH 1/2] Quietly end FastAGI request on handshake disconnect (#49) When a FastAGI client closes before sending the full AGI environment, the handshake read raises AGISIGPIPEHangup from inside _AGIClientHandler.handle(). It propagated uncaught, so socketserver printed a full traceback to stderr for a routine event (caller hung up, Asterisk aborted the leg, or a bare TCP probe). Wrap only the FastAGI(...) handshake construction in except AGIHangup and return early. The suppression is scoped to the environment read, so genuine errors from the script handler, which runs after this guard, still propagate. Add regression tests: a client that closes during the handshake makes handle() return without raising, and a handler that raises still propagates. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 1 + pystrix/agi/fastagi.py | 12 ++++++- tests/test_fastagi_handler.py | 66 +++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 tests/test_fastagi_handler.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 96ddc6e..40aeda1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed - `Manager.monitor_connection` no longer crashes its monitoring thread when the connection drops. Pinging a downed connection raised `ManagerSocketError` (broken socket) or `ManagerError` (the liveness check inside `send_action` failing when the connection dropped just after the loop's own check) inside the thread, which dumped a traceback to stderr and killed the monitor. The monitor now catches both and stops cleanly, logging the reason at debug level when a logger is set. The method also returns the monitoring thread so callers can join it (#3). - `Manager.send_action` no longer raises a raw `AttributeError` when a concurrent `disconnect()` clears the connection between the liveness check and the send. It now drops the just-registered request and raises `ManagerSocketError` instead (#3). +- The FastAGI server no longer logs an unhandled traceback when a client disconnects during the AGI environment handshake. A caller hanging up, Asterisk aborting the leg, or a bare TCP probe raised `AGISIGPIPEHangup` from the handler and printed a full traceback for a routine event. The handler now ends the request quietly. Errors raised by the script handler itself still propagate (#49). ## [1.3.0] - 2026-06-24 diff --git a/pystrix/agi/fastagi.py b/pystrix/agi/fastagi.py index 43bc693..14f723d 100644 --- a/pystrix/agi/fastagi.py +++ b/pystrix/agi/fastagi.py @@ -96,7 +96,17 @@ def handle(self): Creates an instance of an AGI-interface object and passes it to a pre-specified callable, selected by matching the request parameters against a series of regular expressions. """ - agi_instance = FastAGI(self.rfile, self.wfile, debug=self.server.debug) + try: + agi_instance = FastAGI(self.rfile, self.wfile, debug=self.server.debug) + except AGIHangup: + # The client disconnected before sending the full AGI environment: + # the caller hung up as the call reached the AGI step, Asterisk + # aborted the leg, or a bare TCP probe connected and closed. No call + # is in flight, so end the request quietly instead of letting the + # hangup propagate into a socketserver stderr traceback. Errors from + # the handler itself are raised below, outside this guard, so they + # still surface. + return (path, kwargs) = self._extract_query_elements(agi_instance) args = self._extract_positional_args(agi_instance) diff --git a/tests/test_fastagi_handler.py b/tests/test_fastagi_handler.py new file mode 100644 index 0000000..43dcfbd --- /dev/null +++ b/tests/test_fastagi_handler.py @@ -0,0 +1,66 @@ +"""Tests for the FastAGI client handler (`_AGIClientHandler`).""" + +import io +import types + +import pytest + +from pystrix.agi.fastagi import _AGIClientHandler + + +class _ClosedReader: + """The read end of a socket whose client has already disconnected.""" + + def readline(self): + return b"" # EOF: nothing was sent before the connection closed + + +class _EnvReader: + """Yields a minimal AGI environment block, then EOF.""" + + def __init__(self, *lines): + self._lines = list(lines) + self._index = 0 + + def readline(self): + if self._index >= len(self._lines): + return b"" + line = self._lines[self._index] + self._index += 1 + return line + + +def _handler(rfile, handler_callable=None): + # Bypass socketserver's __init__ (which would call handle() itself) and wire + # up only what handle() touches: the read/write files and the server. + instance = _AGIClientHandler.__new__(_AGIClientHandler) + instance.rfile = rfile + instance.wfile = io.BytesIO() + instance.client_address = ("198.51.100.7", 51234) + instance.server = types.SimpleNamespace( + debug=False, + get_script_handler=lambda path: (handler_callable, None), + ) + return instance + + +def test_handle_returns_quietly_when_client_disconnects_during_handshake(): + # Regression for #49: a client that closes before sending the AGI + # environment makes the handshake raise AGISIGPIPEHangup. handle() must end + # the request quietly instead of letting that propagate into a stderr + # traceback from socketserver. + _handler(_ClosedReader()).handle() # must not raise + + +def test_handle_propagates_handler_errors(): + # The handshake-hangup suppression is scoped to the environment read only. + # A genuine error raised by the script handler must still propagate. + def boom(agi, args, kwargs, match, path): + raise RuntimeError("handler blew up") + + handler = _handler( + _EnvReader(b"agi_network_script: demo\n", b"\n"), + handler_callable=boom, + ) + with pytest.raises(RuntimeError): + handler.handle() From d74f5a591199acc32cd23d3f4b0c452ca5d69e32 Mon Sep 17 00:00:00 2001 From: Karthic Raghupathi Date: Wed, 24 Jun 2026 22:24:19 -0400 Subject: [PATCH 2/2] Narrow handshake catch to AGISIGPIPEHangup; strengthen tests (#49 review) Review panel convergence: - Narrow the handler guard from the AGIHangup base to AGISIGPIPEHangup, the only hangup reachable during the environment read (no command is sent, so AGIResultHangup cannot arise). This documents intent precisely, makes the code match the comment/CHANGELOG/test which already name AGISIGPIPEHangup, and avoids silently swallowing a future non-disconnect hangup introduced into construction. - Strengthen the disconnect test: assert the script handler is never invoked and nothing is written back on the quiet path. - Add a mid-handshake disconnect test (partial environment then EOF) covering the parse-loop EOF path, distinct from the first-read EOF case. - CHANGELOG: say the traceback is printed to stderr rather than "logged", since socketserver writes directly to stderr. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 2 +- pystrix/agi/fastagi.py | 10 +++++----- tests/test_fastagi_handler.py | 23 ++++++++++++++++++++++- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40aeda1..dc12401 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed - `Manager.monitor_connection` no longer crashes its monitoring thread when the connection drops. Pinging a downed connection raised `ManagerSocketError` (broken socket) or `ManagerError` (the liveness check inside `send_action` failing when the connection dropped just after the loop's own check) inside the thread, which dumped a traceback to stderr and killed the monitor. The monitor now catches both and stops cleanly, logging the reason at debug level when a logger is set. The method also returns the monitoring thread so callers can join it (#3). - `Manager.send_action` no longer raises a raw `AttributeError` when a concurrent `disconnect()` clears the connection between the liveness check and the send. It now drops the just-registered request and raises `ManagerSocketError` instead (#3). -- The FastAGI server no longer logs an unhandled traceback when a client disconnects during the AGI environment handshake. A caller hanging up, Asterisk aborting the leg, or a bare TCP probe raised `AGISIGPIPEHangup` from the handler and printed a full traceback for a routine event. The handler now ends the request quietly. Errors raised by the script handler itself still propagate (#49). +- The FastAGI server no longer prints an unhandled traceback to stderr when a client disconnects during the AGI environment handshake. A caller hanging up, Asterisk aborting the leg, or a bare TCP probe raised `AGISIGPIPEHangup` from the handler and printed a full traceback for a routine event. The handler now ends the request quietly. Errors raised by the script handler itself still propagate (#49). ## [1.3.0] - 2026-06-24 diff --git a/pystrix/agi/fastagi.py b/pystrix/agi/fastagi.py index 14f723d..8758c99 100644 --- a/pystrix/agi/fastagi.py +++ b/pystrix/agi/fastagi.py @@ -98,14 +98,14 @@ def handle(self): """ try: agi_instance = FastAGI(self.rfile, self.wfile, debug=self.server.debug) - except AGIHangup: - # The client disconnected before sending the full AGI environment: + except AGISIGPIPEHangup: + # The client's pipe closed before the full AGI environment arrived: # the caller hung up as the call reached the AGI step, Asterisk # aborted the leg, or a bare TCP probe connected and closed. No call # is in flight, so end the request quietly instead of letting the - # hangup propagate into a socketserver stderr traceback. Errors from - # the handler itself are raised below, outside this guard, so they - # still surface. + # hangup propagate into a socketserver stderr traceback. This catches + # only the handshake-disconnect signal; errors from the handler + # itself are raised below, outside this guard, so they still surface. return (path, kwargs) = self._extract_query_elements(agi_instance) diff --git a/tests/test_fastagi_handler.py b/tests/test_fastagi_handler.py index 43dcfbd..939d45e 100644 --- a/tests/test_fastagi_handler.py +++ b/tests/test_fastagi_handler.py @@ -49,7 +49,28 @@ def test_handle_returns_quietly_when_client_disconnects_during_handshake(): # environment makes the handshake raise AGISIGPIPEHangup. handle() must end # the request quietly instead of letting that propagate into a stderr # traceback from socketserver. - _handler(_ClosedReader()).handle() # must not raise + invoked = [] + handler = _handler( + _ClosedReader(), handler_callable=lambda *args: invoked.append(args) + ) + + handler.handle() # must not raise + + assert invoked == [] # no script handler runs when the client is already gone + assert handler.wfile.getvalue() == b"" # nothing written back on the quiet path + + +def test_handle_returns_quietly_when_client_disconnects_mid_handshake(): + # A client that sends part of the environment then drops hits EOF inside the + # parse loop, not on the first read. Still a handshake hangup that must end + # the request quietly. + invoked = [] + reader = _EnvReader(b"agi_network_script: demo\n") # no blank terminator, then EOF + handler = _handler(reader, handler_callable=lambda *args: invoked.append(args)) + + handler.handle() # must not raise + + assert invoked == [] def test_handle_propagates_handler_errors():