Updating app reboot logic to make ACL_2_10 test more reliable and consistent (project-chip#43069)

* Updating app reboot logic to make ACL_2_10 test more reliable and consistent

* Re-enabling ACL_2_10 and AVSM tests as the updated reboot logic should make these tests more stable in CI

* Restyled by ruff

* Increasing reboot duration timeout to allow more time for app reboot to complete in CI

* Update test_metadata.yaml

Re-adding AVSM 2 18-21 to verify the current fix for the reboot logic works for ACL_2_10.

* Update test_metadata.yaml

Restoring location of the AVSM tests in test_metadata.yaml to match with master

* Reenabled AVSM tests that were disabled in CI by removing them from the tests_metadata.yaml script after reestablishing multiple reboots scenario again in matter_testing module and validating it worked with the camera app locally, added comment that factory reset is being implemented by Raul in PR 42848, moved allow_multiple_restarts var into if check for restart mode in app reboot monitor, changes timeout var name to timeout_sec

* Restyled by ruff

* Restyled by autopep8

* Resolving linting error with trailing whitespace in test runner

* Updating comment in test runner for factory reset functionality placeholder, and updating value written to restart flag file for restart_once functionality

* Resolving linting error

* Apply suggestions from code review by Andrei

Co-authored-by: Andrei Litvin <andy314@gmail.com>

* Removed logic for single reboot now that the reboot logic holds firmly until after the app-ready-pattern and restart flag file is removed

* Restyled by ruff

* align reset and reboot logic

* Restyled by ruff

* Restyled by autopep8

* Update src/python_testing/matter_testing_infrastructure/matter/testing/matter_testing.py

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* Applying suggestions from Cecille, while adding Raul's logic for if check of reset_type var to gate factory_reset_config_removal call in order to avoidr issues with reboot logic

* Update scripts/tests/run_python_test.py

Co-authored-by: Andrei Litvin <andy314@gmail.com>

---------

Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Andrei Litvin <andy314@gmail.com>
Co-authored-by: Raul Marquez <rmarquez@csa-iot.org>
Co-authored-by: Raul Marquez <130402456+raul-marquez-csa@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Author: 5 people

scripts/tests/run_python_test.py

@@ -245,7 +245,7 @@ def main_impl(app: str, factory_reset: bool, factory_reset_app_only: bool, app_a
     # Remove app config and storage if factory reset is requested
     if factory_reset or factory_reset_app_only:
         reset_type = FactoryResetType.AppAndController if factory_reset else FactoryResetType.AppOnly
-        factory_reset_config_removal(reset_type, app_args, script_args)
+        factory_reset_config_removal(app_args, script_args, reset_type)
 
     app_manager_ref = None
     app_manager_lock = threading.Lock()
@@ -361,6 +361,7 @@ def monitor_app_restart_requests(
         app_manager_lock,
         config: TestRunConfig,
         restart_flag_file):
+
     while True:
         # Try to read the restart flag file
         if not os.path.exists(restart_flag_file):
@@ -370,16 +371,16 @@ def monitor_app_restart_requests(
         with open(restart_flag_file, 'r') as f:
             flag_file_content = f.read().strip()
 
-        # Successfully read the flag file, remove to prevent multiple restarts
-        os.unlink(restart_flag_file)
-        log.info("%s requested by test script", flag_file_content.capitalize())
-
         # Determine reset type and remove app/ctrl config and storage
+        reset_type = None
         if flag_file_content == "factory reset":
             reset_type = FactoryResetType.AppAndController
+
         elif flag_file_content == "factory reset app only":
             reset_type = FactoryResetType.AppOnly
-        factory_reset_config_removal(reset_type, config.app_args, config.script_args)
+
+        if reset_type:
+            factory_reset_config_removal(config.app_args, config.script_args, reset_type)
 
         # Restart the app
         log.info("Restarting app '%s'...", config.app)
@@ -389,6 +390,10 @@ def monitor_app_restart_requests(
             new_app_manager.start()
             app_manager_ref[0] = new_app_manager
 
+        # Successfully read the flag file, remove to prevent multiple restarts
+        os.unlink(restart_flag_file)
+        log.info("%s requested by test script", flag_file_content.capitalize())
+
         # Action complete, continue monitoring for additional restart requests
         log.info("%s completed, continuing to monitor for additional requests", flag_file_content.capitalize())
 
@@ -401,8 +406,8 @@ class FactoryResetType(enum.Enum):
     def config_files(self, app_args: str, script_args: str) -> typing.Generator[str, None, None]:
         """Yield paths of config/storage files to remove for this reset type."""
 
-        # App config files and KVS
-        yield from glob.glob('/tmp/chip*')
+        # App config files and KVS, exclude restart flag file
+        yield from (f for f in glob.glob('/tmp/chip*') if not os.path.basename(f).startswith('chip_test_restart_app'))
         yield from glob.glob('/tmp/repl*')
 
         if match := re.search(r"--KVS (?P<path>[^ ]+)", app_args):
@@ -414,7 +419,7 @@ def config_files(self, app_args: str, script_args: str) -> typing.Generator[str,
                 yield match.group("path")
 
 
-def factory_reset_config_removal(reset_type: FactoryResetType, app_args: str, script_args: str):
+def factory_reset_config_removal(app_args: str, script_args: str, reset_type: FactoryResetType = None):
     """Handles app factory reset requests by removing configuration and storage files."""
     for path in reset_type.config_files(app_args, script_args):
         log.info("Removing config/storage file, path: '%s'...", path)

src/python_testing/matter_testing_infrastructure/matter/testing/matter_testing.py

@@ -1511,15 +1511,18 @@ async def request_device_reboot(self):
         else:
             try:
                 # Create the restart flag file to signal the test runner
+                # Allow for multiple reboots like SW update tests do using the "restart" mode
+                restart_text = "restart"
                 with open(restart_flag_file, "w") as f:
-                    f.write("restart")
+                    f.write(restart_text)
                 LOGGER.info("Created restart flag file to signal app reboot")
 
-                # The test runner will automatically wait for the app-ready-pattern before continuing
-
-                # Expire sessions and re-establish connections
+                # Expire sessions before the monitor picks up the flag
                 self._expire_sessions_on_all_controllers()
-                LOGGER.info("App restart completed successfully")
+
+                await self.wait_for_restart_flag_file_removal(restart_flag_file, restart_text)
+
+                LOGGER.info("App reboot completed successfully")
 
             except Exception as e:
                 LOGGER.error(f"Failed to reboot app: {e}")
@@ -1559,17 +1562,27 @@ async def request_device_factory_reset(self, reset_ctrl: bool = False) -> None:
                     f.write(restart_flag_text)
                     LOGGER.info("Created restart flag file to signal %s request", restart_flag_text)
 
-                # The test runner will automatically wait for the app-ready-pattern before continuing
-
                 # Expire sessions and re-establish connections
                 self._expire_sessions_on_all_controllers()
                 LOGGER.info("%s request sent successfully", restart_flag_text.capitalize())
 
+                await self.wait_for_restart_flag_file_removal(restart_flag_file, restart_flag_text)
+
             except Exception as e:
                 err = f"Failed to {restart_flag_text}: {e}"
                 LOGGER.error(err)
                 asserts.fail(err)
 
+    async def wait_for_restart_flag_file_removal(self, restart_flag_file, restart_flag_text, timeout_sec=30.0):
+        # Wait for the monitor thread to remove the flag file
+        # The monitor deletes the flag file AFTER the restart completes, so this ensures
+        # the app has fully rebooted and is ready before we continue
+        start_time = time.time()
+        while os.path.exists(restart_flag_file):
+            if time.time() - start_time > timeout_sec:
+                asserts.fail(f"App {restart_flag_text} did not complete within timeout (flag file still exists)")
+            await asyncio.sleep(0.1)
+
 
 def _async_runner(body, self: MatterBaseTest, *args, **kwargs):
     """Runs an async function within the test's event loop with a timeout.

src/python_testing/test_metadata.yaml

@@ -13,22 +13,6 @@ not_automated:
       reason: Code/Test not being used or not shared code for any other tests
     - name: TC_AVSMTestBase.py
       reason: Shared code for TC_AVSM, not a standalone test
-    - name: TC_AVSM_2_18.py
-      reason:
-          AVSM flaky test in CI.
-          https://github.com/project-chip/connectedhomeip/issues/42871
-    - name: TC_AVSM_2_19.py
-      reason:
-          AVSM flaky test in CI.
-          https://github.com/project-chip/connectedhomeip/issues/42871
-    - name: TC_AVSM_2_20.py
-      reason:
-          AVSM flaky test in CI.
-          https://github.com/project-chip/connectedhomeip/issues/42871
-    - name: TC_AVSM_2_21.py
-      reason:
-          AVSM flaky test in CI.
-          https://github.com/project-chip/connectedhomeip/issues/42871
     - name: TC_BRBINFO_3_1.py
       reason:
           Attribute is not implemented in the bridge app. Same code as
@@ -210,10 +194,6 @@ not_automated:
       reason:
           Cluster not finalized. Will re-enable before 1.6TE2. CI is not
           supported.
-    - name: TC_ACL_2_10.py
-      reason:
-          Flaky, see
-          https://github.com/project-chip/connectedhomeip/issues/43056
 
 # This is a list of slow tests (just arbitrarily picked around 20 seconds)
 # used in some script reporting for "be patient" messages as well as potentially