From b2e4e5990a342a7173d90030b8e7132e62700408 Mon Sep 17 00:00:00 2001 From: nathant27 Date: Mon, 11 May 2026 20:09:28 -0700 Subject: [PATCH 1/7] fix(amber): avoids false negative on _check_heartbeat when close raises exception --- amber/src/main/python/core/runnables/heartbeat.py | 11 +++++++++-- .../src/test/python/core/runnables/test_heartbeat.py | 9 ++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/amber/src/main/python/core/runnables/heartbeat.py b/amber/src/main/python/core/runnables/heartbeat.py index 1e4c45837d4..70009cc217e 100644 --- a/amber/src/main/python/core/runnables/heartbeat.py +++ b/amber/src/main/python/core/runnables/heartbeat.py @@ -89,12 +89,19 @@ def _check_heartbeat(self) -> bool: temp_socket = socket.create_connection( (self._parsed_server_host, self._parsed_server_port), timeout=1 ) - temp_socket.close() - return True + #temp_socket.close() + #return True except Exception as e: logger.warning(f"Server is down with exception: {e}") return False + try: + temp_socket.close() + except Exception as e: + logger.warning(f"Failed to close socket: {e}") + + return True + @overrides def stop(self): # clean up every process under the python worker diff --git a/amber/src/test/python/core/runnables/test_heartbeat.py b/amber/src/test/python/core/runnables/test_heartbeat.py index 76dc93071aa..4a252fcfd33 100644 --- a/amber/src/test/python/core/runnables/test_heartbeat.py +++ b/amber/src/test/python/core/runnables/test_heartbeat.py @@ -79,12 +79,7 @@ def test_returns_false_when_socket_connection_times_out(self): ): assert hb._check_heartbeat() is False - def test_returns_false_when_socket_close_raises(self): - # Pins the false-negative path: connect succeeds but the subsequent - # close() throws (e.g. broken pipe on a half-open socket). The bare - # `except Exception` in _check_heartbeat() catches it and reports - # "server down", and a regression that narrows that handler would be - # caught here. + def test_returns_true_when_connection_succeeds_but_socket_close_raises(self): hb = make_heartbeat() fake_sock = MagicMock() fake_sock.close.side_effect = OSError("close failed") @@ -92,7 +87,7 @@ def test_returns_false_when_socket_close_raises(self): "core.runnables.heartbeat.socket.create_connection", return_value=fake_sock, ): - assert hb._check_heartbeat() is False + assert hb._check_heartbeat() is True class TestRunEarlyExit: From af58cc53be31ef43c55ce66098649e7e692770b0 Mon Sep 17 00:00:00 2001 From: nathant27 Date: Mon, 11 May 2026 20:31:29 -0700 Subject: [PATCH 2/7] refactor(amber): remove old commented code in _check_heartbeat --- amber/src/main/python/core/runnables/heartbeat.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/amber/src/main/python/core/runnables/heartbeat.py b/amber/src/main/python/core/runnables/heartbeat.py index 70009cc217e..556f4e1e49f 100644 --- a/amber/src/main/python/core/runnables/heartbeat.py +++ b/amber/src/main/python/core/runnables/heartbeat.py @@ -89,8 +89,6 @@ def _check_heartbeat(self) -> bool: temp_socket = socket.create_connection( (self._parsed_server_host, self._parsed_server_port), timeout=1 ) - #temp_socket.close() - #return True except Exception as e: logger.warning(f"Server is down with exception: {e}") return False From 42ff7866042d6e14a1cdd84011df6f2d79186091 Mon Sep 17 00:00:00 2001 From: nathant27 Date: Mon, 11 May 2026 20:49:34 -0700 Subject: [PATCH 3/7] docs: Updated docstring in _check_heartbeat --- amber/src/main/python/core/runnables/heartbeat.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/amber/src/main/python/core/runnables/heartbeat.py b/amber/src/main/python/core/runnables/heartbeat.py index 556f4e1e49f..d76620a0613 100644 --- a/amber/src/main/python/core/runnables/heartbeat.py +++ b/amber/src/main/python/core/runnables/heartbeat.py @@ -79,11 +79,10 @@ def run(self) -> None: def _check_heartbeat(self) -> bool: """ - Attempt to connect to JVM on the specific port. If succeeds, it means the - socket is still available and the JVM is still alive. Otherwise, the JVM - might have been gone. + Attempt to connect to the JVM port. If connection failure, then JVM is dead, or if connection success + then JVM is alive even if close() raises. Logs on connection failure and on close error. - :return: bool, indicating if the socket is available. + :return: bool, True if connect succeeded, False if connect failed. """ try: temp_socket = socket.create_connection( From 71df377e640bf103844bbcd9e1bb009de23f76be Mon Sep 17 00:00:00 2001 From: nathant27 Date: Wed, 13 May 2026 00:48:48 -0700 Subject: [PATCH 4/7] fix: applied ruff reformat on heartbeat.py --- amber/src/main/python/core/runnables/heartbeat.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/amber/src/main/python/core/runnables/heartbeat.py b/amber/src/main/python/core/runnables/heartbeat.py index d76620a0613..cd52a7d091c 100644 --- a/amber/src/main/python/core/runnables/heartbeat.py +++ b/amber/src/main/python/core/runnables/heartbeat.py @@ -79,7 +79,7 @@ def run(self) -> None: def _check_heartbeat(self) -> bool: """ - Attempt to connect to the JVM port. If connection failure, then JVM is dead, or if connection success + Attempt to connect to the JVM port. If connection failure, then JVM is dead, or if connection success then JVM is alive even if close() raises. Logs on connection failure and on close error. :return: bool, True if connect succeeded, False if connect failed. @@ -96,7 +96,7 @@ def _check_heartbeat(self) -> bool: temp_socket.close() except Exception as e: logger.warning(f"Failed to close socket: {e}") - + return True @overrides From 74c5246270588021ef8585c01e6de2e4d7394962 Mon Sep 17 00:00:00 2001 From: nathant27 Date: Wed, 27 May 2026 20:57:11 -0700 Subject: [PATCH 5/7] docs: Updated docstring in check_heartbeat for better clarification on behavior --- amber/src/main/python/core/runnables/heartbeat.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/amber/src/main/python/core/runnables/heartbeat.py b/amber/src/main/python/core/runnables/heartbeat.py index cd52a7d091c..5d88898804b 100644 --- a/amber/src/main/python/core/runnables/heartbeat.py +++ b/amber/src/main/python/core/runnables/heartbeat.py @@ -79,8 +79,8 @@ def run(self) -> None: def _check_heartbeat(self) -> bool: """ - Attempt to connect to the JVM port. If connection failure, then JVM is dead, or if connection success - then JVM is alive even if close() raises. Logs on connection failure and on close error. + Attempt to connect to the JVM port. Returns True iff socket.create_connection succeeds; + a close() failure after a successful connection is logged but DOES NOT flip the return value. :return: bool, True if connect succeeded, False if connect failed. """ From 504f7a4784955f7aa0d17a086390e103cb9ceb39 Mon Sep 17 00:00:00 2001 From: nathant27 Date: Wed, 27 May 2026 21:23:14 -0700 Subject: [PATCH 6/7] test(amber): assert heartbeat closes socket on close error/raises case --- amber/src/test/python/core/runnables/test_heartbeat.py | 1 + 1 file changed, 1 insertion(+) diff --git a/amber/src/test/python/core/runnables/test_heartbeat.py b/amber/src/test/python/core/runnables/test_heartbeat.py index 4a252fcfd33..0fe5b321f50 100644 --- a/amber/src/test/python/core/runnables/test_heartbeat.py +++ b/amber/src/test/python/core/runnables/test_heartbeat.py @@ -88,6 +88,7 @@ def test_returns_true_when_connection_succeeds_but_socket_close_raises(self): return_value=fake_sock, ): assert hb._check_heartbeat() is True + fake_sock.close.assert_called_once() class TestRunEarlyExit: From c4f7b8148792b25a664546f2b94e6c17a97f518a Mon Sep 17 00:00:00 2001 From: nathant27 Date: Wed, 27 May 2026 21:29:28 -0700 Subject: [PATCH 7/7] fix: Fix ruff format check on heartbeat.py --- amber/src/main/python/core/runnables/heartbeat.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/amber/src/main/python/core/runnables/heartbeat.py b/amber/src/main/python/core/runnables/heartbeat.py index 5d88898804b..941782f6998 100644 --- a/amber/src/main/python/core/runnables/heartbeat.py +++ b/amber/src/main/python/core/runnables/heartbeat.py @@ -79,7 +79,7 @@ def run(self) -> None: def _check_heartbeat(self) -> bool: """ - Attempt to connect to the JVM port. Returns True iff socket.create_connection succeeds; + Attempt to connect to the JVM port. Returns True iff socket.create_connection succeeds; a close() failure after a successful connection is logged but DOES NOT flip the return value. :return: bool, True if connect succeeded, False if connect failed.