From 51a00063e78535497216b5dfd9aa388df0112ffd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Sun, 2 Aug 2026 22:09:12 +0200 Subject: [PATCH 1/4] fix(tests): bound exposing_contextmanager's portforward retry loop exposing_contextmanager() retried kubernetes.stream.portforward() forever with no deadline and no give-up condition. Because SocketProxy handles one client synchronously, a single stuck iteration (e.g. a wedged connection to the API server -- the kubernetes client doesn't expose a per-call connect timeout for portforward()) blocks the entire proxy, starving every later Wait.until retry with no way to recover. Add a bounded deadline (default 30s, well under the outer Wait.until 120s budget) that raises TimeoutError once exceeded, and catch it in SocketProxy.listen_and_serve_until_canceled() alongside the existing BrokenPipeError/ConnectionResetError handling so the proxy gives up on just that one connection and keeps serving future retries instead of the exception escaping to the outer handler and killing the whole proxy thread. Also adds the s.close()/pf.close()-before-retry cleanup between iterations (previously each failed attempt leaked the prior PortForward/socket). https://github.com/red-hat-data-services/notebooks/issues/2684 --- tests/containers/kubernetes_utils.py | 31 +++++++++++++++++++++++----- tests/containers/socket_proxy.py | 4 +++- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/tests/containers/kubernetes_utils.py b/tests/containers/kubernetes_utils.py index 60c2b62540..9aaa1aa440 100644 --- a/tests/containers/kubernetes_utils.py +++ b/tests/containers/kubernetes_utils.py @@ -507,21 +507,42 @@ def check_not_none(value: Any, message: str) -> None: @contextlib.contextmanager def exposing_contextmanager( - core_v1_api: kubernetes.client.CoreV1Api, pod: kubernetes.client.models.V1Pod + core_v1_api: kubernetes.client.CoreV1Api, + pod: kubernetes.client.models.V1Pod, + timeout: float = 30, ) -> Generator[socket]: # If we e.g., specify the wrong port, the pf = portforward() call succeeds, # but pf.connected will later flip to False # we need to check that _everything_ works before moving on - pf = None - s = None + # + # https://github.com/red-hat-data-services/notebooks/issues/2684: bound this retry loop so a + # pod that never becomes reachable (or a single kubernetes.stream.portforward() call that + # itself hangs, e.g. on a wedged connection to the API server) can't block the single-threaded + # SocketProxy forever and starve every later Wait.until retry. This bounds the *loop*, not an + # individual hung portforward() call -- the kubernetes client doesn't expose a per-call + # connect timeout for portforward(), only a process-wide `websocket.setdefaulttimeout()`. + deadline = time.monotonic() + timeout + pf: kubernetes.stream.ws_client.PortForward | None = None + s: kubernetes.stream.ws_client.PortForward._Port._Socket | socket.socket | None = None while not pf or not pf.connected or not s: - pf: kubernetes.stream.ws_client.PortForward = kubernetes.stream.portforward( + if time.monotonic() > deadline: + if s is not None: + s.close() + if pf is not None: + pf.close() + raise TimeoutError(f"Failed to establish a working portforward to {pod.metadata.name} within {timeout}s") + if s is not None: + s.close() + s = None + if pf is not None: + pf.close() + pf = kubernetes.stream.portforward( api_method=core_v1_api.connect_get_namespaced_pod_portforward, name=pod.metadata.name, namespace=pod.metadata.namespace, ports=",".join(str(p) for p in [8888]), ) - s: kubernetes.stream.ws_client.PortForward._Port._Socket | socket.socket | None = pf.socket(8888) + s = pf.socket(8888) assert s, "Failed to establish connection" try: diff --git a/tests/containers/socket_proxy.py b/tests/containers/socket_proxy.py index 418c3979a1..9f89074965 100644 --- a/tests/containers/socket_proxy.py +++ b/tests/containers/socket_proxy.py @@ -94,8 +94,10 @@ def listen_and_serve_until_canceled(self): try: # handle client synchronously, which means that there can be at most one at a time self._handle_client(client_socket) - except (BrokenPipeError, ConnectionResetError) as e: + except (BrokenPipeError, ConnectionResetError, TimeoutError) as e: # BrokenPipeError happens when the proxy connects to the pod, but the service inside is not yet listening. + # TimeoutError is raised by remote_socket_factory() (e.g. exposing_contextmanager) when it can't + # establish a working connection within its own bound -- see https://github.com/red-hat-data-services/notebooks/issues/2684. # The client (Wait.until) will retry. logging.info(f"Proxy connection to remote failed, likely due to service not being ready: {e}") # The client_socket is closed by the `with` statement in `_handle_client`. From fb85a53765e5a989190ccb33e2d9efd4e0144e91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Sun, 2 Aug 2026 22:21:46 +0200 Subject: [PATCH 2/4] fixup: don't assume TimeoutError means service-not-ready in proxy log The log message wrapping the caught exception said "likely due to service not being ready", which was accurate for the pre-existing BrokenPipeError/ ConnectionResetError cases but not necessarily for the new TimeoutError (could also mean a genuinely wedged portforward() call, not just a slow-starting pod). Reworded to state what's actually always true instead: the proxy gives up on this one connection and the client will retry. --- tests/containers/socket_proxy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/containers/socket_proxy.py b/tests/containers/socket_proxy.py index 9f89074965..acb98e2d37 100644 --- a/tests/containers/socket_proxy.py +++ b/tests/containers/socket_proxy.py @@ -99,7 +99,7 @@ def listen_and_serve_until_canceled(self): # TimeoutError is raised by remote_socket_factory() (e.g. exposing_contextmanager) when it can't # establish a working connection within its own bound -- see https://github.com/red-hat-data-services/notebooks/issues/2684. # The client (Wait.until) will retry. - logging.info(f"Proxy connection to remote failed, likely due to service not being ready: {e}") + logging.info(f"Proxy connection to remote failed, will retry on the next connection attempt: {e}") # The client_socket is closed by the `with` statement in `_handle_client`. # We continue the loop to accept the next connection attempt. continue From 824c53a4c50018c853dab4ac2e6502c8ce76d622 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Sun, 2 Aug 2026 22:32:44 +0200 Subject: [PATCH 3/4] fixup: bound the individual portforward() call, not just the retry loop CodeRabbit correctly flagged that the deadline check only ran between retry loop iterations: a single kubernetes.stream.portforward() call that itself hangs (the primary scenario this fix targets, e.g. a wedged connection to the API server) would never let the loop's own deadline check run again, silently defeating the bound entirely. The kubernetes client doesn't expose a per-call connect timeout for portforward(), and a process-wide websocket.setdefaulttimeout() isn't safe here (multiple SocketProxy instances can call portforward() concurrently from different threads). Run the call in a helper thread and bound it with join(timeout) instead, via a new _portforward_with_timeout() -- on timeout the call is abandoned (thread/connection leaked for the remainder of the test process) rather than joined, which is an acceptable tradeoff for test infrastructure that's about to fail the test and exit soon after anyway. Verified locally against three scenarios: a portforward() call that hangs forever (previously: hung forever; now: bounded correctly), a pod that fails fast repeatedly (still converges to the loop's own deadline as before), and one that succeeds on a later retry (unaffected). --- tests/containers/kubernetes_utils.py | 65 ++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 13 deletions(-) diff --git a/tests/containers/kubernetes_utils.py b/tests/containers/kubernetes_utils.py index 9aaa1aa440..6cb6a4bffd 100644 --- a/tests/containers/kubernetes_utils.py +++ b/tests/containers/kubernetes_utils.py @@ -505,6 +505,50 @@ def check_not_none(value: Any, message: str) -> None: raise ValueError(message) +def _portforward_with_timeout( + core_v1_api: kubernetes.client.CoreV1Api, pod: kubernetes.client.models.V1Pod, timeout: float +) -> kubernetes.stream.ws_client.PortForward: + """Runs kubernetes.stream.portforward() with a wall-clock bound. + + The kubernetes client doesn't expose a per-call connect timeout for portforward() -- it calls + websocket.connect() with no timeout, so a wedged connection to the API server can hang this call + forever. The only alternative the library offers is a process-wide `websocket.setdefaulttimeout()`, + which isn't safe to use here since multiple SocketProxy instances (one per ImageDeployment) can be + making concurrent portforward() calls from different threads. So we run the call in a helper thread + and bound it with join(timeout) instead. + + If the call doesn't return in time, it is abandoned rather than joined: the daemon thread and any + partially-established connection are leaked for the remainder of the test process. That's an + accepted tradeoff for test infrastructure that's about to raise TimeoutError and fail the test + (and the process will exit soon after) rather than hang forever with no recovery. + """ + pf_result: list[kubernetes.stream.ws_client.PortForward] = [] + error_result: list[Exception] = [] + + def _target() -> None: + try: + pf = kubernetes.stream.portforward( + api_method=core_v1_api.connect_get_namespaced_pod_portforward, + name=pod.metadata.name, + namespace=pod.metadata.namespace, + ports=",".join(str(p) for p in [8888]), + ) + pf_result.append(pf) + except Exception as e: # propagated to the caller below, not swallowed + error_result.append(e) + + thread = threading.Thread(target=_target, daemon=True) + thread.start() + thread.join(timeout) + if error_result: + raise error_result[0] + if not pf_result: + raise TimeoutError( + f"kubernetes.stream.portforward() to {pod.metadata.name} did not return within {timeout:.1f}s" + ) + return pf_result[0] + + @contextlib.contextmanager def exposing_contextmanager( core_v1_api: kubernetes.client.CoreV1Api, @@ -515,17 +559,17 @@ def exposing_contextmanager( # but pf.connected will later flip to False # we need to check that _everything_ works before moving on # - # https://github.com/red-hat-data-services/notebooks/issues/2684: bound this retry loop so a - # pod that never becomes reachable (or a single kubernetes.stream.portforward() call that - # itself hangs, e.g. on a wedged connection to the API server) can't block the single-threaded - # SocketProxy forever and starve every later Wait.until retry. This bounds the *loop*, not an - # individual hung portforward() call -- the kubernetes client doesn't expose a per-call - # connect timeout for portforward(), only a process-wide `websocket.setdefaulttimeout()`. + # https://github.com/red-hat-data-services/notebooks/issues/2684: bound this retry loop (and each + # individual portforward() attempt via _portforward_with_timeout, since a single hung call would + # otherwise never let this loop's own deadline check run again) so a pod that never becomes + # reachable can't block the single-threaded SocketProxy forever and starve every later + # Wait.until retry. deadline = time.monotonic() + timeout pf: kubernetes.stream.ws_client.PortForward | None = None s: kubernetes.stream.ws_client.PortForward._Port._Socket | socket.socket | None = None while not pf or not pf.connected or not s: - if time.monotonic() > deadline: + remaining = deadline - time.monotonic() + if remaining <= 0: if s is not None: s.close() if pf is not None: @@ -536,12 +580,7 @@ def exposing_contextmanager( s = None if pf is not None: pf.close() - pf = kubernetes.stream.portforward( - api_method=core_v1_api.connect_get_namespaced_pod_portforward, - name=pod.metadata.name, - namespace=pod.metadata.namespace, - ports=",".join(str(p) for p in [8888]), - ) + pf = _portforward_with_timeout(core_v1_api, pod, remaining) s = pf.socket(8888) assert s, "Failed to establish connection" From e691e4a21471a10c6b0089bacf2a01ba994a66dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Sun, 2 Aug 2026 22:40:09 +0200 Subject: [PATCH 4/4] fixup: apply ruff format (previous commit missed socket_proxy.py) --- tests/containers/kubernetes_utils.py | 2 +- tests/containers/socket_proxy.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/containers/kubernetes_utils.py b/tests/containers/kubernetes_utils.py index 6cb6a4bffd..ddb86f1bb8 100644 --- a/tests/containers/kubernetes_utils.py +++ b/tests/containers/kubernetes_utils.py @@ -518,7 +518,7 @@ def _portforward_with_timeout( and bound it with join(timeout) instead. If the call doesn't return in time, it is abandoned rather than joined: the daemon thread and any - partially-established connection are leaked for the remainder of the test process. That's an + partially established connection are leaked for the remainder of the test process. That's an accepted tradeoff for test infrastructure that's about to raise TimeoutError and fail the test (and the process will exit soon after) rather than hang forever with no recovery. """ diff --git a/tests/containers/socket_proxy.py b/tests/containers/socket_proxy.py index acb98e2d37..de3a130eb9 100644 --- a/tests/containers/socket_proxy.py +++ b/tests/containers/socket_proxy.py @@ -99,7 +99,9 @@ def listen_and_serve_until_canceled(self): # TimeoutError is raised by remote_socket_factory() (e.g. exposing_contextmanager) when it can't # establish a working connection within its own bound -- see https://github.com/red-hat-data-services/notebooks/issues/2684. # The client (Wait.until) will retry. - logging.info(f"Proxy connection to remote failed, will retry on the next connection attempt: {e}") + logging.info( + f"Proxy connection to remote failed, will retry on the next connection attempt: {e}" + ) # The client_socket is closed by the `with` statement in `_handle_client`. # We continue the loop to accept the next connection attempt. continue