You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bryancall (via GitHub)" <gi...@apache.org> on 2026/10/13 21:11:59 UTC
[PR] Fix h2_to_origin_safe_retry flakes from abrupt origin closes [trafficserver]
bryancall opened a new pull request, #13834:
URL: https://github.com/apache/trafficserver/pull/13834
`h2_to_origin_safe_retry`, added in [#13447](https://github.com/apache/trafficserver/pull/13447), fails now and then in autest shard 1of4, usually with a 502 to the client. The failures come from how the test's Python origin, `safe_retry_origin.py`, ends its connections, not from ATS.
The origin closed its socket as soon as it wrote its last frame. ATS often has a SETTINGS ACK in flight at that point, and closing a socket that still holds unread input makes Linux reset the connection, which can drop the response the origin just wrote. The goaway, goaway-reserved, goaway-drain-above and early-rst retries failed this way on a busy machine. The origin now shuts down its sending side and reads until ATS closes before it closes the socket. It does this on a background thread so it can still accept the new connection ATS opens for a retry.
The early-rst origin also closes its connection right after the reset. ATS can take that session back from the pool for the retry before it reads the close, and it then correctly declines to replay a POST written to a connection that closed without a response. That scenario now sets `server_session_sharing.match` to `none`, so its retry always opens a new connection. The other scenarios keep `ip` matching and still cover a retry on a reused connection.
Testing: in the `ci.trafficserver.apache.org/ats/fedora:44` image, with 32 copies of the test running in parallel, master failed 39 of 640 runs on one machine, and 135 of 800 on a second machine with CPU load added. With this change it failed 0 of 640 and 0 of 800, although ATS admitted the early-rst retry before reading the origin's close in 596 and 437 of those runs. To check that the test still catches a broken retry, I built ATS so it never admits a not-processed retry. The changed test failed on that build in all 4 runs, on the same 12 checks as the original test.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix h2_to_origin_safe_retry flakes from abrupt origin closes [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall merged PR #13834:
URL: https://github.com/apache/trafficserver/pull/13834
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix h2_to_origin_safe_retry flakes from abrupt origin closes [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13834:
URL: https://github.com/apache/trafficserver/pull/13834#discussion_r4265051272
##########
tests/gold_tests/h2/safe_retry_origin.py:
##########
@@ -122,6 +122,26 @@ def _wait_for_close(self, tls_socket: ssl.SSLSocket) -> None:
print("drained_session_closed_by_peer", flush=True)
return
+ def finish_connection(self, tls_socket: ssl.SSLSocket) -> None:
+ """Half-close and read until ATS closes, so that closing cannot reset the connection.
+
+ Closing a socket that still holds unread input, such as a SETTINGS ACK
+ that ATS sent after the last read, makes the kernel reset the connection,
+ which can discard what this origin just wrote, such as the retried response.
+ """
+ try:
+ tls_socket.shutdown(socket.SHUT_WR)
+ except OSError:
+ return
+ while not self.lifecycle.stopped:
+ try:
+ if not tls_socket.recv(65535):
+ return
+ except TimeoutError:
+ continue
+ except OSError:
+ return
Review Comment:
Won't fix. `ssl.SSLZeroReturnError`, `ssl.SSLEOFError` and every other `ssl.SSLError` subclass `OSError`, so the `except OSError` already catches them. `SSLSocket.shutdown()` also drops the SSL object before it half-closes, so the reads after it are plain socket reads.
##########
tests/gold_tests/h2/safe_retry_origin.py:
##########
@@ -253,19 +278,25 @@ def serve_socket(plain_socket: socket.socket, listener: socket.socket | None) ->
# Ignore readiness probes and peers that never finish TLS.
plain_socket.close()
return
- with tls_socket:
+ try:
if tls_socket.selected_alpn_protocol() != "h2":
raise RuntimeError("ATS did not negotiate HTTP/2 with the origin")
tls_socket.settimeout(0.2)
origin.serve_connection(tls_socket, listener)
+ except BaseException:
+ tls_socket.close()
+ raise
+ # Finish in the background: ATS can keep a finished connection open
+ # while it opens the next one, which this origin must accept.
+ threads.append(threading.Thread(target=finish_socket, args=(tls_socket,)))
Review Comment:
Won't fix. `run_server` joins every thread in `threads` before it returns, finishers included. Each one reads with a 0.2 s timeout and checks `lifecycle.stopped` on every pass, so it exits within 0.2 s of the stop file and cannot hold up shutdown.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix h2_to_origin_safe_retry flakes from abrupt origin closes [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13834:
URL: https://github.com/apache/trafficserver/pull/13834#discussion_r4264230398
##########
tests/gold_tests/h2/safe_retry_origin.py:
##########
@@ -122,6 +122,26 @@ def _wait_for_close(self, tls_socket: ssl.SSLSocket) -> None:
print("drained_session_closed_by_peer", flush=True)
return
+ def finish_connection(self, tls_socket: ssl.SSLSocket) -> None:
+ """Half-close and read until ATS closes, so that closing cannot reset the connection.
+
+ Closing a socket that still holds unread input, such as a SETTINGS ACK
+ that ATS sent after the last read, makes the kernel reset the connection,
+ which can discard what this origin just wrote, such as the retried response.
+ """
+ try:
+ tls_socket.shutdown(socket.SHUT_WR)
+ except OSError:
+ return
+ while not self.lifecycle.stopped:
+ try:
+ if not tls_socket.recv(65535):
+ return
+ except TimeoutError:
+ continue
+ except OSError:
+ return
Review Comment:
`finish_connection()` uses `SSLSocket.recv()` but doesn’t handle TLS EOF / SSL-layer exceptions (e.g., `ssl.SSLZeroReturnError`, `ssl.SSLEOFError`, or more general `ssl.SSLError`). `_wait_for_close()` already treats these as a clean close; here they would escape the thread and can surface as noisy unhandled exceptions (and potentially affect test stability). Align the exception handling with `_wait_for_close()` by treating TLS EOF as `data = b''` / returning, and optionally handling `ConnectionResetError` similarly.
##########
tests/gold_tests/h2/safe_retry_origin.py:
##########
@@ -253,19 +278,25 @@ def serve_socket(plain_socket: socket.socket, listener: socket.socket | None) ->
# Ignore readiness probes and peers that never finish TLS.
plain_socket.close()
return
- with tls_socket:
+ try:
if tls_socket.selected_alpn_protocol() != "h2":
raise RuntimeError("ATS did not negotiate HTTP/2 with the origin")
tls_socket.settimeout(0.2)
origin.serve_connection(tls_socket, listener)
+ except BaseException:
+ tls_socket.close()
+ raise
+ # Finish in the background: ATS can keep a finished connection open
+ # while it opens the next one, which this origin must accept.
+ threads.append(threading.Thread(target=finish_socket, args=(tls_socket,)))
Review Comment:
These finisher threads are created as non-daemon threads by default. If `run_server()` shutdown does not reliably join all threads in `threads`, the process can hang waiting for finisher threads (especially if ATS keeps the connection open until near shutdown). Ensure these finisher threads are either marked daemon or are always joined during shutdown (including in error paths).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix h2_to_origin_safe_retry flakes from abrupt origin closes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13834:
URL: https://github.com/apache/trafficserver/pull/13834#discussion_r4316414952
##########
tests/gold_tests/h2/safe_retry_origin.py:
##########
@@ -253,19 +278,25 @@ def serve_socket(plain_socket: socket.socket, listener: socket.socket | None) ->
# Ignore readiness probes and peers that never finish TLS.
plain_socket.close()
return
- with tls_socket:
+ try:
if tls_socket.selected_alpn_protocol() != "h2":
raise RuntimeError("ATS did not negotiate HTTP/2 with the origin")
tls_socket.settimeout(0.2)
origin.serve_connection(tls_socket, listener)
+ except BaseException:
+ tls_socket.close()
+ raise
+ # Finish in the background: ATS can keep a finished connection open
+ # while it opens the next one, which this origin must accept.
+ threads.append(threading.Thread(target=finish_socket, args=(tls_socket,)))
+ threads[-1].start()
Review Comment:
[P2] Start the thread you created rather than the shared list's last entry
In goaway-split, serve_socket runs in workers while the accept loop also appends and starts entries in threads. If a worker appends its finisher here, the accept loop then appends and starts another connection worker, and the first worker resumes at threads[-1].start(), it tries to start that other worker a second time. Its own finisher never starts, so shutdown can also fail when joining it. I reproduced this interleaving with the unchanged run_server function; it raises RuntimeError: cannot join thread before it is started. Keep a local reference to each new Thread and start that object, both here and in the accept loop at lines 309-310, so concurrent appends cannot change the selected thread.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix h2_to_origin_safe_retry flakes from abrupt origin closes [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13834:
URL: https://github.com/apache/trafficserver/pull/13834#discussion_r4316653203
##########
tests/gold_tests/h2/safe_retry_origin.py:
##########
@@ -253,19 +278,25 @@ def serve_socket(plain_socket: socket.socket, listener: socket.socket | None) ->
# Ignore readiness probes and peers that never finish TLS.
plain_socket.close()
return
- with tls_socket:
+ try:
if tls_socket.selected_alpn_protocol() != "h2":
raise RuntimeError("ATS did not negotiate HTTP/2 with the origin")
tls_socket.settimeout(0.2)
origin.serve_connection(tls_socket, listener)
+ except BaseException:
+ tls_socket.close()
+ raise
+ # Finish in the background: ATS can keep a finished connection open
+ # while it opens the next one, which this origin must accept.
+ threads.append(threading.Thread(target=finish_socket, args=(tls_socket,)))
+ threads[-1].start()
Review Comment:
Good catch, fixed in 81dc033e4a. Both places now create the thread, start it through a local reference, and only then append it to `threads`, so a concurrent append cannot change which thread gets started, and the join loop never sees a thread that has not started.
To check it, I paused the worker just before its start line so the accept loop could append and start another worker in that gap. The old code failed 50 of 50 runs with `RuntimeError: cannot join thread before it is started`, and the new code passed 50 of 50. The full `h2_to_origin_safe_retry` test also passed 64 of 64 runs in the Fedora 44 CI image under CPU load.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org