fixed review comments H7-H12

Author: Saumya-R

feature_integration_tests/test_cases/daemon_helpers.py

@@ -105,17 +105,14 @@ def find_binary_in_runfiles(target_path: str) -> Path | None:
     return None
 
 
-def get_binary_path(target: str, version: str = "rust") -> Path:
+def get_binary_path(target: str) -> Path:
     """
     Get path to a binary, either from runfiles or by building it.
 
     Parameters
     ----------
     target : str
         Bazel target path.
-    version : str
-        Build version ("rust" or "cpp").
-
     Returns
     -------
     Path
@@ -411,7 +408,7 @@ def launch_manager_daemon(
 
     # Get launch_manager daemon binary (from runfiles or build it)
     daemon_target = "@score_lifecycle_health//src/launch_manager_daemon:launch_manager"
-    daemon_binary = get_binary_path(daemon_target, version)
+    daemon_binary = get_binary_path(daemon_target)
 
     # Copy daemon to bin directory
     daemon_path = bin_dir / "launch_manager"
@@ -424,7 +421,7 @@ def launch_manager_daemon(
         ("@score_lifecycle_health//examples/cpp_supervised_app:cpp_supervised_app", "cpp_supervised_app"),
         ("@score_lifecycle_health//examples/control_application:control_daemon", "control_daemon"),
     ]:
-        app_binary = get_binary_path(app_target, version)
+        app_binary = get_binary_path(app_target)
         app_dest = bin_dir / app_name
         shutil.copy2(app_binary, app_dest)
         app_dest.chmod(0o755)

feature_integration_tests/test_cases/tests/lifecycle/test_process_launching_with_daemon.py

@@ -96,7 +96,7 @@ def setup_test_app(self, launch_manager_daemon: dict[str, Any], version: str) ->
             app_target = "@score_lifecycle_health//examples/cpp_supervised_app:cpp_supervised_app"
             app_name = "cpp_supervised_app"
 
-        app_binary = get_binary_path(app_target, version)
+        app_binary = get_binary_path(app_target)
 
         # Copy to daemon bin directory
         dest_path = bin_dir / app_name
@@ -150,16 +150,12 @@ def test_supervised_app_launches(
             )
             print(f"\nDaemon reported supervised process PID events: {', '.join(pid_events[:5])}")
 
-        # Verify daemon is still running
+        # Verify daemon is still running and target app appears in supervision logs.
         assert daemon.is_running(), "Launch Manager daemon stopped unexpectedly"
-
-        # Check daemon logs for supervision activity
         logs = daemon.get_logs()
-        print(f"\nDaemon logs:\n{logs}")
-
-        # Verify logs show component was started
-        # (Actual log messages depend on Launch Manager implementation)
-        assert len(logs) > 0, "No daemon logs generated"
+        assert app_name in logs or result.returncode == 0, (
+            f"No target-specific supervision evidence for {app_name}.\nDaemon logs:\n{logs}"
+        )
 
     def test_supervised_app_recovery(
         self, launch_manager_daemon: dict[str, Any], setup_test_app: Path, version: str
@@ -195,36 +191,40 @@ def test_supervised_app_recovery(
                 print(f"\nKilling supervised app (PID: {pid})")
                 subprocess.run(["kill", "-9", pid], check=True)
 
-                # Wait for daemon to detect failure and restart
-                time.sleep(2.0)
-
-                # Verify app was restarted
-                result = subprocess.run(
-                    ["pgrep", "-f", app_name],
-                    capture_output=True,
-                    text=True,
-                    check=False,
-                )
-
-                if result.returncode == 0:
-                    new_pid = result.stdout.strip().split("\n")[0]
+                # Poll for restart to avoid fixed-sleep race conditions.
+                deadline = time.time() + 12.0
+                new_pid = None
+                while time.time() < deadline:
+                    result = subprocess.run(
+                        ["pgrep", "-f", app_name],
+                        capture_output=True,
+                        text=True,
+                        check=False,
+                    )
+                    if result.returncode == 0:
+                        candidates = [p for p in result.stdout.strip().split("\n") if p and p != pid]
+                        if candidates:
+                            new_pid = candidates[0]
+                            break
+                    time.sleep(0.25)
+
+                if new_pid is not None:
                     print(f"App restarted with new PID: {new_pid}")
-                    assert new_pid != pid, "PID should be different after restart"
                 else:
                     logs = daemon.get_logs()
                     pytest.fail(
-                        "Supervised app was not found after forced kill; expected daemon recovery restart."
-                        f"\nDaemon logs:\n{logs}"
+                        "Supervised app was not restarted within timeout after forced kill."
+                        f"\nTarget app: {app_name}\nDaemon logs:\n{logs}"
                     )
             else:
                 logs = daemon.get_logs()
-                recovery_signals = [
-                    f"unexpected termination of process",
-                    "Activating Recovery state.",
-                    f"Got kRunning timeout for process",
+                target_patterns = [
+                    rf"unexpected termination of process.*\(\s*{re.escape(app_name)}\s*\)",
+                    rf"Got kRunning timeout for process.*\(\s*{re.escape(app_name)}\s*\)",
                 ]
-                assert any(signal in logs for signal in recovery_signals), (
-                    f"Supervised app {app_name} not running and no recovery diagnostics found.\nDaemon logs:\n{logs}"
+                assert any(re.search(pattern, logs) for pattern in target_patterns), (
+                    f"Supervised app {app_name} not running and no target-specific recovery diagnostics found."
+                    f"\nDaemon logs:\n{logs}"
                 )
 
         except subprocess.CalledProcessError as e:
@@ -264,16 +264,42 @@ def test_watchdog_detection(self, launch_manager_daemon: dict[str, Any], version
         3. Validate recovery action is triggered
         """
         daemon = launch_manager_daemon["daemon"]
+        app_name = "rust_supervised_app" if version == "rust" else "cpp_supervised_app"
 
-        # Give daemon enough time to perform supervision cycles and emit diagnostics.
-        time.sleep(2.0)
-        logs = daemon.get_logs()
-
-        watchdog_like_signals = [
-            "Got kRunning timeout for process",
-            "Problem discovered in PG MainPG Activating Recovery state.",
-            "unexpected termination of process",
-        ]
-        assert any(signal in logs for signal in watchdog_like_signals), (
-            "No supervision/watchdog-related diagnostics found in daemon logs."
+        # Setup: stop the supervised process to emulate a non-reporting workload.
+        result = subprocess.run(
+            ["pgrep", "-f", app_name],
+            capture_output=True,
+            text=True,
+            check=False,
         )
+        if result.returncode != 0:
+            # Some CI environments cannot keep startup apps alive due to uid/gid
+            # switching restrictions. Do not skip: require target-specific
+            # watchdog/recovery evidence from daemon logs instead.
+            logs = daemon.get_logs()
+            watchdog_patterns = [
+                rf"Got kRunning timeout for process.*\(\s*{re.escape(app_name)}\s*\)",
+                rf"unexpected termination of process.*\(\s*{re.escape(app_name)}\s*\)",
+            ]
+            assert any(re.search(pattern, logs) for pattern in watchdog_patterns), (
+                f"Target app {app_name} is not running and no target-specific watchdog diagnostics were found."
+                f"\nDaemon logs:\n{logs}"
+            )
+            return
+
+        pid = result.stdout.strip().split("\n")[0]
+        subprocess.run(["kill", "-STOP", pid], check=True)
+        try:
+            # Allow supervision/watchdog loop to detect stalled process.
+            time.sleep(4.0)
+            logs = daemon.get_logs()
+            watchdog_patterns = [
+                rf"Got kRunning timeout for process.*\(\s*{re.escape(app_name)}\s*\)",
+                rf"unexpected termination of process.*\(\s*{re.escape(app_name)}\s*\)",
+            ]
+            assert any(re.search(pattern, logs) for pattern in watchdog_patterns), (
+                f"No target-specific watchdog diagnostics found for {app_name}.\nDaemon logs:\n{logs}"
+            )
+        finally:
+            subprocess.run(["kill", "-CONT", pid], check=False)

feature_integration_tests/test_scenarios/cpp/src/scenarios/lifecycle/launch_manager_support.cpp

@@ -62,27 +62,25 @@ struct LifecycleTestInput {
 
         const auto& test = test_object_res.value().get();
 
-        // Initialize with sensible defaults to prevent zero-initialization issues
-        LifecycleTestInput input{100, 3};
-
         const auto duration_it = test.find("test_duration_ms");
-        if (duration_it != test.end()) {
-            const auto duration_res = duration_it->second.As<uint64_t>();
-            if (duration_res.has_value()) {
-                input.test_duration_ms = duration_res.value();
-            }
+        if (duration_it == test.end()) {
+            throw std::invalid_argument("Missing required field: test_duration_ms");
+        }
+        const auto duration_res = duration_it->second.As<uint64_t>();
+        if (!duration_res.has_value()) {
+            throw std::invalid_argument("Field test_duration_ms must be an unsigned integer");
         }
 
         const auto count_it = test.find("checkpoint_count");
-        if (count_it != test.end()) {
-            const auto count_res = count_it->second.As<uint64_t>();
-            // Validate checkpoint_count >= 1 to prevent division by zero
-            if (count_res.has_value() && count_res.value() >= 1U) {
-                input.checkpoint_count = static_cast<size_t>(count_res.value());
-            }
+        if (count_it == test.end()) {
+            throw std::invalid_argument("Missing required field: checkpoint_count");
+        }
+        const auto count_res = count_it->second.As<uint64_t>();
+        if (!count_res.has_value()) {
+            throw std::invalid_argument("Field checkpoint_count must be an unsigned integer");
         }
 
-        return input;
+        return LifecycleTestInput{duration_res.value(), static_cast<size_t>(count_res.value())};
     }
 };
 
@@ -213,7 +211,7 @@ class ParallelLaunching : public Scenario {
             throw std::runtime_error("checkpoint_count must be at least 1");
         }
 
-        std::cout << "Testing parallel execution pattern with multiple monitors" << std::endl;
+        std::cout << "Testing parallel health monitoring with multiple monitors" << std::endl;
 
         std::cout << "Started " << std::to_string(test_input.checkpoint_count)
                   << " parallel monitors" << std::endl;
@@ -534,22 +532,20 @@ class ConditionalLaunching : public Scenario {
         }
 
         std::cout << "Testing conditional launching" << std::endl;
-        if (!wait_conditions.empty()) {
-            for (const auto& condition : wait_conditions) {
-                if (condition.rfind("path:", 0) == 0U) {
-                    std::cout << "Checking path condition: " << condition.substr(5) << std::endl;
-                } else if (condition.rfind("env:", 0) == 0U) {
-                    std::cout << "Checking env condition: " << condition.substr(4) << std::endl;
-                } else if (condition.rfind("process:", 0) == 0U) {
-                    std::cout << "Checking process condition: " << condition.substr(8) << std::endl;
-                } else {
-                    std::cout << "Checking condition: " << condition << std::endl;
-                }
+        if (wait_conditions.empty()) {
+            throw std::runtime_error("Wait conditions were not provided: missing 'test.wait_conditions' in scenario input");
+        }
+
+        for (const auto& condition : wait_conditions) {
+            if (condition.rfind("path:", 0) == 0U) {
+                std::cout << "Checking path condition: " << condition.substr(5) << std::endl;
+            } else if (condition.rfind("env:", 0) == 0U) {
+                std::cout << "Checking env condition: " << condition.substr(4) << std::endl;
+            } else if (condition.rfind("process:", 0) == 0U) {
+                std::cout << "Checking process condition: " << condition.substr(8) << std::endl;
+            } else {
+                throw std::runtime_error("Unsupported wait condition prefix: " + condition);
             }
-        } else {
-            std::cout << "Checking path condition: /tmp/ready" << std::endl;
-            std::cout << "Checking env condition: STARTUP_COMPLETE" << std::endl;
-            std::cout << "Checking process condition: init_done" << std::endl;
         }
         std::cout << "Polling interval: " << polling_interval << "ms" << std::endl;
         std::cout << "Condition timeout: " << timeout << "ms" << std::endl;

feature_integration_tests/test_scenarios/rust/src/scenarios/lifecycle/launch_manager_support.rs

@@ -340,17 +340,28 @@ impl Scenario for ConditionalLaunching {
 
         info!("Testing conditional launching");
 
-        if let Some(conditions) = v["test"]["wait_conditions"].as_array() {
-            for condition in conditions {
-                if let Some(cond_str) = condition.as_str() {
-                    if cond_str.starts_with("path:") {
-                        info!("Checking path condition: {}", &cond_str[5..]);
-                    } else if cond_str.starts_with("env:") {
-                        info!("Checking env condition: {}", &cond_str[4..]);
-                    } else if cond_str.starts_with("process:") {
-                        info!("Checking process condition: {}", &cond_str[8..]);
-                    }
-                }
+        let conditions = v["test"]["wait_conditions"].as_array().ok_or_else(|| {
+            "Wait conditions were not provided: missing 'test.wait_conditions' in scenario input".to_string()
+        })?;
+
+        if conditions.is_empty() {
+            return Err(
+                "Wait conditions were not provided: empty 'test.wait_conditions' in scenario input".to_string(),
+            );
+        }
+
+        for condition in conditions {
+            let cond_str = condition
+                .as_str()
+                .ok_or_else(|| "Wait condition entries must be strings".to_string())?;
+            if cond_str.starts_with("path:") {
+                info!("Checking path condition: {}", &cond_str[5..]);
+            } else if cond_str.starts_with("env:") {
+                info!("Checking env condition: {}", &cond_str[4..]);
+            } else if cond_str.starts_with("process:") {
+                info!("Checking process condition: {}", &cond_str[8..]);
+            } else {
+                return Err(format!("Unsupported wait condition prefix: {}", cond_str));
             }
         }