From 7a363fbc8507d8c8dc532927766f10518d2b286e Mon Sep 17 00:00:00 2001
From: glados-verma <akashv@google.com>
Date: Thu, 18 Dec 2025 13:09:49 -0800
Subject: [PATCH] Fix bug in test executor where manual aborts were not letting
 the test clean up properly.

To understand this bug, note that:

1. OpenHTF registers a signal handler for SIGINT
2. This handler re-raises KeyboardInterrupt
3. The `execute` function in TestDescriptor expects this, and effectively waits on the test runner thread to join - twice in the case of a manual abort.

This would have worked until [Python was "fixed"](https://github.com/python/cpython/commit/a22be4943c119fecf5433d999227ff78fc2e5741#diff-b250d3d0e0d909784a67083e9069ce507513a67e8514a011e2f364027e25e0b0). (Equivalent code doesn't exist in Python 3.14, but at least today, in Python 3.12, this bites us). After this, the second wait() call would return immediately while the thread was still running. is_alive() also indicates the thread is dead when in fact it's still alive.

It's debatable whether the "fix" above is the right thing, but we can avoid this hairy issue by not using `Thread.join()` and instead use `Event.wait()` which we can explicitly set at the point we want the thread to be considered "done".

Without this fix, the test executor thread is still running teardowns while the output callbacks have started processing in the main thread, causing output callbacks to receive a test descriptor that hasn't been finalized - among other issues.

PiperOrigin-RevId: 846387423
---
 openhtf/core/test_descriptor.py | 10 ++++++++++
 openhtf/core/test_executor.py   | 16 +++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/openhtf/core/test_descriptor.py b/openhtf/core/test_descriptor.py
index a017643c7..5c8b8d8dd 100644
--- a/openhtf/core/test_descriptor.py
+++ b/openhtf/core/test_descriptor.py
@@ -258,6 +258,8 @@ def handle_sig_int(cls, signalnum: Optional[int], handler: Any) -> None:
       test.abort_from_sig_int()
     if not cls.HANDLED_SIGINT_ONCE:
       cls.HANDLED_SIGINT_ONCE = True
+      # Re-raise the KeyboardInterrupt in the main thread so that any other
+      # handlers aren't oblivious to this having happened.
       raise KeyboardInterrupt
     # Otherwise, does not raise KeyboardInterrupt to ensure that the tests are
     # cleaned up.
@@ -340,7 +342,15 @@ def trigger_phase(test):
     except KeyboardInterrupt:
       # The SIGINT handler only raises the KeyboardInterrupt once, so only retry
       # that once.
+      _LOG.info(
+          'Waiting for clean interrupted exit from test: %s',
+          self.descriptor.code_info.name,
+      )
       self._executor.wait()
+      _LOG.info(
+          'Clean interrupted exit from test: %s',
+          self.descriptor.code_info.name,
+      )
       raise
     finally:
       try:
diff --git a/openhtf/core/test_executor.py b/openhtf/core/test_executor.py
index 634af20dc..d5a0d2b5d 100644
--- a/openhtf/core/test_executor.py
+++ b/openhtf/core/test_executor.py
@@ -110,6 +110,7 @@ def __init__(self, test_descriptor: 'test_descriptor.TestDescriptor',
     self._last_execution_unit: str = None
     self._abort = threading.Event()
     self._full_abort = threading.Event()
+    self._execution_finished = threading.Event()
     # This is a reentrant lock so that the teardown logic that prevents aborts
     # affects nested sequences.
     self._teardown_phases_lock = threading.RLock()
@@ -165,15 +166,13 @@ def abort(self) -> None:
   def finalize(self) -> test_state.TestState:
     """Finalize test execution and output resulting record to callbacks.
 
-    Should only be called once at the conclusion of a test run, and will raise
-    an exception if end_time_millis is already set.
+    Should only be called once at the conclusion of a test run.
 
     Returns:
       Finalized TestState.  It must not be modified after this call.
 
     Raises:
-      TestStopError: test
-      TestAlreadyFinalized if end_time_millis already set.
+      TestStopError: If the test is already stopped or never ran.
     """
     if not self.test_state:
       raise TestStopError('Test Stopped.')
@@ -193,10 +192,16 @@ def wait(self) -> None:
         threading.TIMEOUT_MAX,
         31557600,  # Seconds in a year.
     )
-    self.join(timeout)
+    # This function is expected to be called twice in the case of a SIGINT,
+    # and in Python 3.12 the second call would always return immediately,
+    # preventing a clean exit (see `execute` in test_descriptor.py). Instead,
+    # we wait on an Event that we control.
+    self._execution_finished.wait(timeout)
+    self.join()
 
   def _thread_proc(self) -> None:
     """Handles one whole test from start to finish."""
+    self._execution_finished.clear()
     try:
       # Top level steps required to run a single iteration of the Test.
       self.test_state = test_state.TestState(self._test_descriptor, self.uid,
@@ -228,6 +233,7 @@ def _thread_proc(self) -> None:
       raise
     finally:
       self._execute_test_teardown()
+      self._execution_finished.set()
 
   def _initialize_plugs(
       self,